[Part 9]: Introducing Sink::close #159
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
An explicit close is required because in the new model (Titus), we want the ability to stop tasks gracefully. This means shutting down all the resources that the job is currently consuming. In the old world (Apache Mesos), this was not required because we were forcefully shutting down processes which are both not clean as well as not feasible in the new world.
sundargates
requested review from
codyrioux,
nickmahilani,
jeffchao,
piygoyal,
calvin681,
Andyz26,
hmitnflx,
markcho and
liuml07
as code owners
Andyz26
approved these changes
Apr 4, 2022
| public void close() throws IOException { | ||
| if (pushServerSse != null) { | ||
| pushServerSse.shutdown(); | ||
| } else if (httpServer != null) { |
There was a problem hiding this comment.
nit: in this case IMO null check is enough to shutdown pushServerSse and httpServer, which is slightly safer if for any reason both of them got created.
There was a problem hiding this comment.
yea I'm not too worried about it honestly given that the code for setting these fields is just before this method. so even if that changes in the future when you create both the servers for whatever reason, you can easily change the behavior of the close() since it's in the same file.
codyrioux
approved these changes
Apr 4, 2022
mantis-runtime/src/main/java/io/mantisrx/runtime/sink/Sinks.java
Outdated
Show resolved
Hide resolved
|
It should be an error state if both somehow got created.
…On Mon, Apr 4, 2022 at 9:58 AM Andy Zhang ***@***.***> wrote:
***@***.**** approved this pull request.
------------------------------
In
mantis-runtime/src/main/java/io/mantisrx/runtime/sink/ServerSentEventsSink.java
<#159 (comment)>:
> }
portObservable.onNext(port);
}
+ @OverRide
+ public void close() throws IOException {
+ if (pushServerSse != null) {
+ pushServerSse.shutdown();
+ } else if (httpServer != null) {
nit: in this case IMO null check is enough to shutdown pushServerSse and
httpServer, which is slightly safer if for any reason both of them got
created.
—
Reply to this email directly, view it on GitHub
<#159 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACUD75NF3ZG7DS4P2ZHSUDVDMNS7ANCNFSM5SLCWPCQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
sundargates
commented
Apr 4, 2022
Comment on lines
+69
to
+72
| sink.close(); | ||
| } finally { | ||
| subscription.unsubscribe(); | ||
| } |
There was a problem hiding this comment.
@calvin681 should we unsubscribe before doing the close?
There was a problem hiding this comment.
TBH, I'm not even sure why we need to eager subscribe to the observable in call. Regardless, unsubscribing first would make more logical sense.
sundargates
commented
Apr 4, 2022
Comment on lines
+85
to
+91
| @Override | ||
| public void close() throws IOException { | ||
| try { | ||
| sink.close(); | ||
| } finally { | ||
| subscription.unsubscribe(); | ||
| } |
sundargates
commented
Apr 4, 2022
Comment on lines
+92
to
+96
| try { | ||
| sink.close(); | ||
| } finally { | ||
| subscription.unsubscribe(); | ||
| } |
There was a problem hiding this comment.
should the unsubscribe happen before sink::close?
sundargates
commented
Apr 4, 2022
Comment on lines
+81
to
+85
| try { | ||
| sink.close(); | ||
| } finally { | ||
| subscription.unsubscribe(); | ||
| } |
There was a problem hiding this comment.
Same as before.
|
let me merge the change now. Any pending comments will be addressed in subsequent diffs. |
calvin681
approved these changes
Apr 4, 2022
sundargates
added a commit
to sundargates/mantis
that referenced
this pull request
Apr 21, 2022
* [Part 9]: Introducing Sink::close An explicit close is required because in the new model (Titus), we want the ability to stop tasks gracefully. This means shutting down all the resources that the job is currently consuming. In the old world (Apache Mesos), this was not required because we were forcefully shutting down processes which are both not clean as well as not feasible in the new world. Co-authored-by: Sundaram Ananthanarayanan <sananthanarayanan@netflix.com> Intermediate commit Intermediate commit Intermediate commit Intermediate commit ignoring the failing test fixing the failing tests Fixing failures Adding example job jar ignoring the failing test everyting is there Changing the prefixes for the configurations ignoring tests for faster builds Adding extra logging Ensuring FileSystem is getting initialized Fixing the rpc system loader for prod Forgot to start high availability services cleaning up cleaning up Adding more logs adding some extra logs Fixing deserialization issues adding some extra logs Revert "adding some extra logs" This reverts commit 9756aa2. formatting Increasing certain timeouts Cleaning up some code Moved Ack to mantis-common fixing formatting issues fixing the way the rpc system is loaded in both the server and the task executor Some fixes updating Revert "Disabling checkstyle errors" This reverts commit 8af1bb1. getting rid of check.gradle updating actions adding more routes cleaning up Making ExecuteStageRequest serializable Cleaned up the code fixing test Fixing bugs in scheduler changing the way scheduler gets used Adding some extra logs commit Making sure parameter is serializable more serializable adding more logs Logging Fixing a few things Added some tests adding more logs updating version Code cleanup updating deps adding more logs fixing one more thing adding more logs fixing one more thing fixing one more thing fixing one more thing fixing one more thing Cleaning up workerports fixing one more thing updating deps updating deps changing the source and sinks so that they close themselves cleanly changing the ordering in which the publisher and consumer are closed updating deps load parent mantis classes first moving to parent class loader changing the thread name getting the code to compile after rebase fixes after rebase local commit minor commit minor commit minor commit deleting unnecessary files deleting unnecessary files deleting unnecessary files deleting unnecessary files Updates deleting unnecessary files deleting unnecessary files deleting unnecessary files Compilation successful getting rid of unnecessary changes getting rid of unnecessary changes deleting unnecessary files deleting unnecessary files Fixing some issues getting rid of unnecessary changes getting rid of unnecessary changes Changing something getting rid of unnecessary changes
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.
An explicit close is required because in the new model (Titus), we want the ability to stop tasks gracefully. This means shutting down all the resources that the job is currently consuming. In the old world (Apache Mesos), this was not required because we were forcefully shutting down processes which are both not clean as well as not feasible in the new world.
Context
We are introducing a breaking API change because we want to ensure developers don't forget to close their sinks correctly.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all testsCONTRIBUTING.md