YARN-9425. Make initialDelay configurable for FederationStateStoreService#scheduledExecutorService #4731
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
…vice#scheduledExecutorService
slfan1989
approved these changes
Aug 11, 2022
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Aug 12, 2022
There was a problem hiding this comment.
Makes sense to me. Thanx @ashutoshcipher, Give a check if you can extend a test as well. If you don't find any smart way to check the functionality, Can just check on the LOG messages. GenericTestUtils.LogCapturer can help with that.
Comment on lines
3923
to
3929
| public static final String FEDERATION_STATESTORE_HEARTBEAT_INITIAL_DELAY_SECS = | ||
| FEDERATION_PREFIX + "state-store.heartbeat.initial-delay-secs"; | ||
|
|
||
| // 30 secs | ||
| public static final int | ||
| DEFAULT_FEDERATION_STATESTORE_HEARTBEAT_INITIAL_DELAY_SECS = 30; | ||
|
|
There was a problem hiding this comment.
Make the config support rather than binding it just with Seconds
There was a problem hiding this comment.
Thanks @ayushtkn for reviewing. Can you please elaborate on this? Are you suggesting to remove -secs and _SECS?
There was a problem hiding this comment.
yes, don't bind it with seconds, just make seconds as the default type, in case no unit is specified use Seconds, else whatever unit is specified. conf.getTimeDuration should do it for you.
Check this if it help:
Comment on lines
128
to
131
| if (heartbeatInitialDelay <= 0) { | ||
| heartbeatInitialDelay = | ||
| YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_HEARTBEAT_INITIAL_DELAY_SECS; | ||
| } |
There was a problem hiding this comment.
Add a warn log here, that the configured value for the Config is wrong it should be greater than 0, so using the default of....
And somewhere below an info log that we are using an initial delay of {configured} {unit}
There was a problem hiding this comment.
Makes sense. I will address this in my next commit.
|
💔 -1 overall
This message was automatically generated. |
Failure is unrelated, Ran it multiple times in local - its working fine. |
|
@ayushtkn - I have addressed your comments, can you please review it again. Thanks. |
ayushtkn
approved these changes
Aug 16, 2022
| TimeUnit.SECONDS); | ||
|
|
||
| if (heartbeatInitialDelay <= 0) { | ||
| LOG.warn("{} configured value is wrong, must be at <= 0; using default value of {}", |
There was a problem hiding this comment.
must be at <=0? It should be greater than 0, right?
There was a problem hiding this comment.
Thanks for pointing it out. Sorry I missed it. Fixed it in latest commit.
Thanks for review and approval :)
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @ayushtkn - Thanks for your review and approval, I have update the log line as well. Can you commit this |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…vice#scheduledExecutorService (apache#4731). Contributed by groot and Shen Yinjie. Signed-off-by: Ayush Saxena <ayushsaxena@apache.org>
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 of PR
Make initialDelay configurable for FederationStateStoreService#scheduledExecutorService
JIRA - YARN-9425
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?