This repository has been archived by the owner on Apr 16, 2022. It is now read-only.
Pull & push ops overhaul #736
Merged
Conversation
This file contains 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
|
Hi! This is marked as a draft. Shall I start looking at it anyway? Will there likely be many more changes? |
|
Yes!, just a couple more commits to restore HTTP proxy and add more tests coverage. I was hoping to get into a live review session with you. We can discuss on slack if you want |
ggalmazor
commented
May 9, 2019
ggalmazor
commented
May 9, 2019
| public JPanel container; | ||
| private SourcePanel sourcePanel; | ||
| private SourcePanelForm sourcePanelForm; | ||
| private SourceOrTargetPanel<T> sourceOrTargetPanel; |
There was a problem hiding this comment.
Using "SourceOrTarget" from now on for lack of a better name
|
I realize some commit messages fall somewhat short to explain some whys and that I'd like to make some small changes. I'll be rehashing the commits during my day so that you can have better material in your day, @dcbriccetti. I'll give you a heads up when it's done. |
- The build process (at least when done in IntelliJ) will complain about using the methods managed by the UI Designer tool outside the form classes. - What we would really want is to use form class instances themselves as containers, but I haven't found a way to do this yet.
- Now the TransferPanelForm has a type parameter to set the kind of panel it's hosting (pull or push). - Now that the Source class has been segregated into the PushSource and PullTarget classes, this type parameter lets us continue having generic UI components in a typesafe way
- Review language in Request, RequestBuilder & Response classes to disambiguate concepts - Base Request-Response on InputStreams instead of Strings for improved performance (avoid reprocessing responses as much as possible) and smaller memory footprint on big HTTP interactions - The RequestBuilder now allows creating POST requests with multipart support, and new response mappers to JSON, and XmlElement - The Response object includes more info from non success responses, required for the ODK Central integration
- Replace reusing/non-reusing conns with max conn number in Http - We always want to reuse connections, so having a factory that doesn't it makes no sense - We want (in the future) to limit the number of concurrent connections - Implement POST requests that can send a single or multipart message payload - For the time being, hardcode concurrent connection number to 8 (1 for tests)
- Add new ways to compose and sequence Jobs - Improve the JobsRunner's API to deal with type safety in a simpler way, with the tradeoff of losing the builder style API. - Also simpified launching Jobs by losing the type parameter in JobsRunner (effectively making it a void operation) to cope with the complexity of futures and sync/async processes, which we don't need to deal with for now. Instead, call sites need to declare callbacks that will be called on success/failure.
- Move pull classes to an aggregate subpackage
- The key change in this commit is the one that declares a new Cursor arg in InstanceIdBatchGetter to be used as the starting cursor, which allows us to pass one when launching the pull operation. - Also replace string primitives with Cursor instances to make explicit the important business relation between cursors and getting submission lists from Aggregate - We make Cursors required for low level operation such as getting the submission id pages/batches/chunks - We make Cursors optional for the high level push operation to let calling site decide which cursor to use, either the last saved cursor or a synthetic cursos built using a date provided by the user
- Rename RemoteServer to AggregateServer and extract RemoteServer interface
We are setting the model for a pull/push process: - One low-level of abstraction individual method for each HTTP interaction - It has to be aware of the runner status - It has to do error management - It has to do logging & user feedback - It has to be explicit about side-effects by returning void where possible - It has to have unit/integration tests - One high-level of abstraction method that composes the full operation: - It has to compose Jobs that call the individual low-level methods - There has to be as much small Jobs as possible - The composition has to be coherent with the return type of the low-level methods: supply (non-void) vs run/accept (void) - It has to have unit/integration tests
- The implementation uses assets from the Export vertical package that should be moved to a reused horizontal package.
- The implementation uses assets from the Export vertical package that should be moved to a reused horizontal package.
- Move the decision of how to launch the request to the calling site to decouple the operation from the context where it's executed - Other calling sites might make the decision to launch it synchronously instead
- We control the flow with the JobsRunner API now
- We need this in case the user wants to pull/push forms in bulk. Otherwise, it wouldn't be possible to know to which forms each tracking event message belongs to
- Change "username" to "email", which is more appropriate
- It should say "Success"
- We need to be able to limit HTTP connections in the push operation as well
- For conformity: we did the same thing in TransferForms - Now we achieve the same things with less indirection - *Forms classes are right to handle state that they own (selection of forms, export confs, etc.), but they turn to be bad when dealing with pull sources, which they don't own because it's a cross-cutting concept in our domain
- We want to use fresh credentials every time that we need to create a new connection to a server - Apache HTTPClient stores credentials in two ways: - There's a credentials provider that saves used credentials before they're used - There's an auth cache that saves used auth schemes - By disabling auth cache and cleaning the auth provider, we ensure that we will be using the provided credentials each time a new connection is established.
- Simplify JobsRunner by replacing the onSuccess callback with composition of the success action into the individual jobs - Normalize side-effects of individual jobs and groups of jobs: - tracking event sending: (Pull|Push)Event.Success is sent by the individual jobs, (Pull|Push)Event.Complete is sent by the pull|push operation (when all jobs have completed) - The export panel unsets the "exporting mode" when all exports have ended - Review and document what the JobsRunner onError callback does
- Now we read from the app prefs and the tab (local) prefs, giving preference to what we read on the app prefs, which is the "new way to do it"
- Detected an issue with the design of JobsRunner around waitForCompletion and onComplete. Added TODO comments with enough context to retake this in the future
- This way we avoid making requests that are probably going to fail anyway and simplify error handling in the last big block
- By throwing a named exception, we can handle it downstream in a controlled way. - We need to handle known exceptions like this one and let only "unexpected exceptions" reach the JobsRunner
- Extract reading & storing methods into RemoteServer and Cursor respectively and reuse everywhere - RemoteServer read methods get both the app prefs and the pull panel prefs in order to try to read from the old location at the pull panel prefs - Create code regions for prefs management in Cursor and RemoteServer - Ensure that we only deal with one instance of prefs object for the pull panel class
(when combined with the "start form last" feature9
- CLI ops need to be able to set a different briefcase dir (-sd arg) instead of using BriefcasePreference objects to get it. - Otherwise, one can't run the CLI without having set an sd through the UI, and changing to different storage dirs won't be clean (Briefcase will think that forms pulled into one sd are present in another sd)
This was referenced Jun 26, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
v1.6 QA feedback
Work for the future
Differences between Aggregate and Central
New issues
Pending stuff
Guillermo's To Do list
-pllawithout specifying a form. Use Kasia's Aggregate server. She reports having pulled only 7 from 60 available forms.-sfl. Kasia reports-sfdworks as expected, so the best guess is that cursors aren't being saved correctly.-psha. Kasia reports:-e -pb. Kasia reports that no file is created in the export dir.Solved stuff
-ppflag in pull ops-mhc(max HTTP conns) flag has been enabled and can be used with pull ops.Added stuff
pulldataxform functionStuff we have learned