Handle leaking TP exception in handleAssignmentChange#845
Merged
surajkn merged 1 commit intolinkedin:masterfrom Aug 20, 2021
Merged
Handle leaking TP exception in handleAssignmentChange#845surajkn merged 1 commit intolinkedin:masterfrom
surajkn merged 1 commit intolinkedin:masterfrom
Conversation
vmaheshw
requested changes
Jul 29, 2021
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
c9c58ab to
e9006d2
Compare
surajkn
commented
Aug 2, 2021
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
vmaheshw
requested changes
Aug 3, 2021
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
...-server/src/main/java/com/linkedin/datastream/server/DummyTransportProviderAdminFactory.java
Show resolved
Hide resolved
datastream-server-restli/src/test/java/com/linkedin/datastream/server/TestCoordinator.java
Show resolved
Hide resolved
e9006d2 to
b862840
Compare
surajkn
commented
Aug 3, 2021
datastream-server-restli/src/test/java/com/linkedin/datastream/server/TestCoordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
b862840 to
77a0c31
Compare
vmaheshw
reviewed
Aug 6, 2021
Collaborator
vmaheshw
left a comment
There was a problem hiding this comment.
Some code refactoring comments. One thought related to load on zookeeper server after this change.
| * @param throwOnSend whether or not to throw an exception on send calls | ||
| */ | ||
| public DummyTransportProviderAdminFactory(boolean throwOnSend) { | ||
| public DummyTransportProviderAdminFactory(boolean throwOnSend, boolean failTransportProvider) { |
Collaborator
There was a problem hiding this comment.
a. change failTransportProvider to failTransportProviderOnce
b. update javadocs.
| } | ||
| }); | ||
|
|
||
| int totalTasks = currentAssignment.values().stream().mapToInt(List::size).sum(); |
Collaborator
There was a problem hiding this comment.
nit: You can extract method out of this logic.
| if ((totalTasks - submittedTasks) > 0) { | ||
| _log.warn("Failed to submit {} tasks from currentAssignment. Queueing onAssignmentChange event again", totalTasks - submittedTasks); | ||
| // Update the metric and queue the event only once for all the tasks that failed to be initialized | ||
| if (isDatastreamUpdate) { |
Collaborator
There was a problem hiding this comment.
Qn: Why do you need a different code logic for this v/s TimeoutException, when the expectation is the same in both the cases.
_eventQueue.put(CoordinatorEvent.createHandleDatastreamChangeEvent());
} else {
_eventQueue.put(CoordinatorEvent.createHandleAssignmentChangeEvent());
}
throw e;
``` There is no direct exception known in this path, but you can create DatastreamRuntimeException(). If you extract the highlighted code into a method and call it, it may work for you.
Logically it is the same code, but different way of achieving the same thing.
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Show resolved
Hide resolved
77b03cf to
30e908a
Compare
vmaheshw
previously requested changes
Aug 12, 2021
Collaborator
vmaheshw
left a comment
There was a problem hiding this comment.
Some cosmetic changes and make the retry field configurable.
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
datastream-server/src/main/java/com/linkedin/datastream/server/Coordinator.java
Outdated
Show resolved
Hide resolved
9322769 to
5e5e49b
Compare
During handleAssignmentChange, while initializing task we were leaking a possible exception while creating TransportProvider object. To address it we catch the exception for the failed task and mark that task failed and prevent it from being added to assigned tasks and also queue another handle assignment change event. Also introducing a maximum retry count for onAssignementChange in case of errors.
5e5e49b to
8724e19
Compare
shrinandthakkar
approved these changes
Aug 17, 2021
jzakaryan
approved these changes
Aug 17, 2021
vmaheshw
pushed a commit
to vmaheshw/brooklin
that referenced
this pull request
Mar 1, 2022
During handleAssignmentChange, while initializing task we were leaking a possible exception while creating TransportProvider object. To address it we catch the exception for the failed task and mark that task failed and prevent it from being added to assigned tasks and also queue another handle assignment change event. Also introducing a maximum retry count for onAssignementChange in case of errors.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
During handleAssignmentChange, while initializing task we were leaking a possible
exception while creating TransportProvider object. This patch fixes it by issuing
a retry on failure.
Important: DO NOT REPORT SECURITY ISSUES DIRECTLY ON GITHUB.
For reporting security issues and contributing security fixes,
please, email security@linkedin.com instead, as described in
the contribution guidelines.
Please, take a minute to review the contribution guidelines at:
https://github.com/linkedin/Brooklin/blob/master/CONTRIBUTING.md