-
-
Notifications
You must be signed in to change notification settings - Fork 37
Media Upload Tools #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
CocoisBuggy
wants to merge
12
commits into
develop
Choose a base branch
from
feat/#443
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Media Upload Tools #465
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The GCS service account key (if present)
(untested) Mongodb alleges it can manage TTL on a document by document basis so we will be trying this to elide stale media that doesn't get uploaded to the bucket
I have chosen for the time being to support either the PUSH or PULL based subscribers, and for now I haven't done anything clever with deletion events though realistically we could do that to make sure media gets properly removed - though it would require some authentication hoops to be jumped through. Ideally I'd like for us to choose one of these that we would like to support and then leave the other at the wayside, opting for a mock service to be used with all other development environments.
The tests related to full integration will skip unless the environment signals that it is properly instrumented for integratrion tests. The full tests require wiring up to a TLS-backed endpoint and all the GCS infrastructure set up ahead of the test.
Oops, I left them as example values which causes the instrumentation tests to run rather than skip
This should indicate that the new features can be safely built on these interfaces
We leave it to the client to recall its own promised media, because the alternative involves convuluting the GQL schema and potentially poisoning the consistency. In the event that promised media gets dropped, so what? the server will statelessly deal with the timeout by quietly terminating the pending media. This means we need a test case for re-submitting an idential pending media promise. In this case I imagine we can yield back the existing pending object - unless the expiry is imminent in which case we can re-sign a url and send it back.
As far as I can tell these are quite comprehensive, I need to run a realworld test of the google pull subscriber load balancing so that I can see if the complexity is needless. That will be the next commit. This commit contains working code that should be safe to integrate into the existing services quite easily so long as the infrastructure has the notifications configured along with it. I've tried to make the tests catch any funny business, so it should be safe for any developer to come and poke around without disrupting anything. The integration tests are the end-all since if they pass, the code should be totally funcitonal.
The tests were trying to run with full GCS service access when the user has no credentials.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Working a little bit on #443, and this is where my thinking is taking me so far.
There isn't consensus on this being the best approach - the alternate is discussed in the issue comment chain - but I wanted to take a look and make sure my proposal was feasible
I have written code and tests for both subscription paradigms that google supports, but I guess realistically we need to choose one and curb the other one. @vnugent I'm not really an expert in GCS, but as I see it the choice is down to how we do load balancing.
My initial intuition was that we should just do a web hook since then the requests from google will flow through whatever balancing strategy is in place, but I see some evidence that actually the subscribers get a subset of the messages and that some load balancing is already in place. Do you have any idea? We can go either way since both options are implemented and tested
Next step is just to update the gql mutation and add some tests and then the feature should be done. We can roll it out without affecting any of the existing media upload features in opentacos.
I've put some extra documentation in the relevant README here