TEZ-4349. DAGClient gets stuck with invalid cached DAGStatus #161
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
|
🎊 +1 overall
This message was automatically generated. |
abstractdog
reviewed
Nov 14, 2021
| dagClientRpc.setAMProxy(createMockProxy(DAGStatusStateProto.DAG_SUCCEEDED, 1000l)); | ||
| dagClientRpc.injectAMFault(new IOException("injected AM Fault")); | ||
| dagClient.resetCounters(); | ||
| dagClientRpc.resetCountesr(); |
Contributor
There was a problem hiding this comment.
could you please fix this typo?
(I know it's not introduced with this patch, but it's worth fixing as you already working on this area)
abstractdog
reviewed
Nov 14, 2021
| if (cachedDAG != null) { | ||
| // could not get from AM (not reachable/ was killed). return cached status. | ||
| return cachedDagStatus; | ||
| return cachedDAG; |
Contributor
There was a problem hiding this comment.
am I right to assume this is the codepath where the original issue happened? could you please clarify how can we indefinitely stuck here?
I mean, we can only hit this part if getDAGStatusViaAM returns null but dagCompleted is not true, so when we hit this again and again in getDAGStatusViaAM :
} catch (TezException e) {
// can be either due to a n/w issue of due to AM completed.
} catch (IOException e) {
// can be either due to a n/w issue of due to AM completed.
}
also getApplicationReportInternal keeps returning null in checkAndSetDagCompletionStatus
was it the case for you?
if so, does it make sense to put at least debug level log messages to the silent catch branches?
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback.
Yes, considering the implementation of TezJob.run()-Line222 in Pig:
Pig polls on the DAGStatus inside an infinite loop:
while (true) {
try {
dagStatus = dagClient.getDAGStatus(null);
} catch (Exception e) {
log.info("Cannot retrieve DAG status", e);
break;
}
if (dagStatus.isCompleted()) {
// do something
// break;
}
sleep(1000);
} Let's assume the following scenario on Tez Side:
- Pig first iteration calls
getDAGStatusViaAM()which successfully pulls the DAGStatus and updates the cachedDAGStatus to running. - Pig sleeps 1000
- second call from Pig calls
getDAGStatusViaAM()which encountersTezExceptionorIOException. The call would return the last cachedDAGStatus (which is running), instead of null. - Since the status is
running, the Pig-thread sleeps - This will keep going as long as the
getDAGStatusViaAM()fails, and the last valid DAGStatus is still cached.
The problem in this corner case is that the Pig client will keep looping indefinitely as long as it does not receive a null or dagClient.getDAGStatus(null) does not throw an exception.
From a client perspective, it is better to fail early in order to recover faster.
|
🎊 +1 overall
This message was automatically generated. |
abstractdog
reviewed
Nov 15, 2021
| public static final String TEZ_CLIENT_DAG_STATUS_CACHE_TIMEOUT_MINUTES = TEZ_PREFIX | ||
| + "client.dag.status.cache.timeout-minutes"; | ||
| // Default timeout is 5 minutes. | ||
| public static final long TEZ_CLIENT_DAG_STATUS_CACHE_TIMEOUT_MINUTES_DEFAULT = 5; |
Contributor
There was a problem hiding this comment.
I think we can lower this to 1 minute, especially because in case of query status stuck (that the patch is about to address) this default value will be the minimum time in which the client has the chance to realize the stuck cached progress
Contributor
There was a problem hiding this comment.
I'm wondering if we actually would prefer seconds. I can see use cases that would benefit from a lower time than 1 minute.
Contributor
Author
There was a problem hiding this comment.
I am fine with seconds. However, I am not so sure about the impact of setting the expiration to a small value.
For example, it is possible to have some delay in the AM executing the RPC call (or even a timeout at one time). If the expiration of the cache is smaller than the total of ("AM RPC with failure" + "wait for the client to retry" + "AM RPC"), then the cache won't serve its purpose.
If I understand correctly, cachedDAGStatus purpose is to protect the client from falling too soon to the RM. Correct me if I misunderstand the purpose of the cachedDAGStatus.
Contributor
Author
There was a problem hiding this comment.
The latest commit changes the TimeUnit to seconds and the default to 60.
Contributor
There was a problem hiding this comment.
thanks @amahussein, I'm assuming the default value 60s would work properly
|
🎊 +1 overall
This message was automatically generated. |
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.
TEZ-4349 DAGClient gets stuck with invalid cached DAGStatu
The
cachedDagStatusshould be valid for a certain amount of time, or certain number of retires.When the
cachedDAGStatusexpires, the DAGClient tries to pull from AM or the RM.An error in fetching the status from both AM and RM, would return null to the caller.
TezConfiguration.TEZ_CLIENT_DAG_STATUS_CACHE_TIMEOUT_MINUTEStez.client.dag.status.cache.timeout-minutes, and the default is 5. ThetimeUnitof the expiration is minutes.TestDAGClient.testGetDagStatusWithCachedStatusExpirationmvn test -Dtest=TestDAGClient,TestTezClient,TestMockDAGAppMaster