New async bindings #614
Merged
New async bindings #614
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
A memory leak for errantly formed http messages going through HttpByteBufFormatter; log4j2 config bug that stopped old replayer logs from compressing. I've also further improved the logging on shutdown. Signed-off-by: Greg Schohn <[email protected]>
1) Netty futures are now bound to CompletableFutures via a utility function that has the netty future simply mark a CompletableFuture<Void> as finished (with the null value) or propagate the exception into the CompletableFuture. That allows the rest of the codebase to leverage CompletableFuture and DiagnosticTrackableCompletableFuture objects and methods rather than a cobbled approach. 2) Helper classes that managed coordination and scheduling (TimeToResponseFulfillmentFutureMap and OnlineRadixSorter) have been redesigned to be more functional so that future values can be derived from earlier values rather than rely upon side effects to connect the dots. Some tests still don't pass and intense testing will be required before these changes are better than what was previously there. Signed-off-by: Greg Schohn <[email protected]>
…h issues in 'slowTest' Signed-off-by: Greg Schohn <[email protected]>
Signed-off-by: Greg Schohn <[email protected]> # Conflicts: # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayer.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayerCore.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/datatypes/TimeToResponseFulfillmentFutureMap.java
…FulfillmentFutureMap to use a dequeue instead of a queue. If all of the events for the time fulfillment map come in order thanks to the sequencer, there's no reason to use a more complicated/expensive data structure. When the items are in order, they lend themselves more to chaining too. Also... Refactor netty-CompletableFuture binding helpers into a new utility class. Open up the DiagnosticTrackableCompletableFuture class to make it easier to test that chains are properly formed and updated. Start to build out the RequestSenderOrchestratorTest a little bit more. Signed-off-by: Greg Schohn <[email protected]>
… to allow for chaining. In addition to chaining sequential stages together (which now, again, need to create all intermediates before the current work item), the class now emits which sequential stages are missing (outstanding) in the DCF's diagnostic supplier. Sample output may look like this [504336483] Caller-task completion for idx=3[…]<-[335107734] OnlineRadixSorterForIntegratedKeys.addFutureForWork[…]<-[215078753] Kickoff for slot #3[…]<-[1384454980] Work to finish for slot #2 is awaiting [slotsOutstanding:2][…] Signed-off-by: Greg Schohn <[email protected]>
…ies. Cull grandparents (if they exist) when the current future is done, checking when traversing the ancestry chain and in a whenComplete handler (which may be pretty late to the mix given how many other dependent functions might be in line before it). There's also now the ability to set the dependent parent AFTER construction. This helps to bind sequential stages from the OnlineRadixSorter together. Signed-off-by: Greg Schohn <[email protected]>
Signed-off-by: Greg Schohn <[email protected]> # Conflicts: # TrafficCapture/trafficReplayer/src/test/java/org/opensearch/migrations/replay/RequestSenderOrchestratorTest.java
gregschohn
requested review from
AndreKurait,
chelma,
kartg,
lewijacn,
mikaylathompson,
okhasawn and
sumobrian
as code owners
AndreKurait
reviewed
Apr 26, 2024
...eplayer/src/test/java/org/opensearch/migrations/replay/e2etests/FullTrafficReplayerTest.java
Outdated
Show resolved
Hide resolved
...trafficReplayer/src/main/java/org/opensearch/migrations/NettyToCompletableFutureBinders.java
Outdated
Show resolved
Hide resolved
...trafficReplayer/src/main/java/org/opensearch/migrations/NettyToCompletableFutureBinders.java
Outdated
Show resolved
Hide resolved
...rafficReplayer/src/main/java/org/opensearch/migrations/replay/RequestSenderOrchestrator.java
Show resolved
Hide resolved
...rafficReplayer/src/main/java/org/opensearch/migrations/replay/RequestSenderOrchestrator.java
Show resolved
Hide resolved
| return consumeFuture.thenCompose(cf -> | ||
| NettyToCompletableFutureBinders.bindNettyScheduleToCompletableFuture(eventLoop, | ||
| Duration.between(now(), startAt.plus(interval.multipliedBy(counter.get())))) | ||
| .getDeferredFutureThroughHandle((v,t)-> sendSendingRestOfPackets(packetReceiver, eventLoop, |
There was a problem hiding this comment.
Should this be getDeferredFutureThroughCompose to fast fail?
There was a problem hiding this comment.
Maybe (thenCompose)? I'm trying that. A netty schedule operation will fail during shutdown & I don't see why I wouldn't want to fail faster in that situation. The other call is already in a thenCompose, so the fastness of the failure is already pretty quick, but I think that you're right that it could still be quicker.
...e/trafficReplayer/src/main/java/org/opensearch/migrations/replay/util/OnlineRadixSorter.java
Outdated
Show resolved
Hide resolved
…ingText shorter by doing run-length compression. Signed-off-by: Greg Schohn <[email protected]>
The old format is still present and is used to print outstanding work that's outstanding when the replayer is shut down. Json should make it easier to pretty print the dependency graph. I think it might make more sense to re-reverse the order of the graph for json output though. I should also shift the LEVEL line to the beginning so that it's easier to copy-paste. Signed-off-by: Greg Schohn <[email protected]>
…letableFutureBinders + other assorted tweaks. I've also stripped the longRunningActivity file of the log levels and timestamps. They were really just getting in the way of understanding what was going on and being able to quickly copy-paste from there into a json file for further analysis. Signed-off-by: Greg Schohn <[email protected]>
…be shorter. s/DiagnosticTrackableCompletableFuture/TrackedFuture/ s/NettyToCompletableFutureBinders/NettyFutureBinders/ Identifier names were also updated. Where types could be implicitly deduced, code was updated so that these type names didn't need to be specified. Signed-off-by: Greg Schohn <[email protected]>
AndreKurait
approved these changes
Apr 29, 2024
…e and we go to close it Signed-off-by: Greg Schohn <[email protected]>
4 tasks
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.
Description
These changes begin to bind netty Futures to CompletableFutures by using a proxy CompletableFuture (and DiagnosticTrackableCompletableFuture wrapper) that simply signaled asynchronously from the addListener callback. From there, the promise graphs can be extended to be much more complete. This change attempts to fill out much more of the promise graph so that we can diagnose why a workflow would hang. Below is a sample of a top-level DiagnosticTrackableCompletableFuture, which used to render simply as something closer to...
Issues Resolved
[List any issues this PR will resolve]
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.