HBASE-24628 Region normalizer now respects a rate limit (HMaster chore shutdown NPE ADDENDUM) #2540
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
ndimiduk
requested review from
saintstack,
Apache9,
huaxiangsun and
virajjasani
saintstack
approved these changes
Oct 13, 2020
Apache9
requested changes
Oct 14, 2020
| @@ -1638,7 +1638,9 @@ private void stopChores() { | |||
| choreService.cancelChore(this.mobFileCleanerChore); | |||
| choreService.cancelChore(this.mobFileCompactionChore); | |||
| choreService.cancelChore(this.balancerChore); | |||
| choreService.cancelChore(getRegionNormalizerManager().getRegionNormalizerChore()); | |||
| Optional.ofNullable(this.regionNormalizerManager) | |||
There was a problem hiding this comment.
Why not just use a null check?
There was a problem hiding this comment.
Optional.ofNullable() is invoked twice, one here and another within mapper:
public<U> Optional<U> map(Function<? super T, ? extends U> mapper) {
Objects.requireNonNull(mapper);
if (!isPresent())
return empty();
else {
return Optional.ofNullable(mapper.apply(value));
}
}
And ofNullable() itself is going to check if value is null, and accordingly going to create new Optional object.
When we have direct Function to deal with, this approach would come with minimal complexity. I am not saying here it is too complex, but compared to one simple null check, yes it is bit complex :)
|
And in general, I think the design here is a bit strange. If we define RegionNormalizerManager as a separated feature, then we should schedule chore and cancel it in RegionNormalizerManager itself, in the start and stop methods. If not, we should do things like CatalogJanitor, where the class itself is a chore, so in HMaster we will schedule and cancel it. Anyway, let's see if this could fix the problem first. |
|
OK, tried locally, a simple null check could fix the problem. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
My objective was to make a normalizer subsystem that's as self-contained as possible. The rough edges are related to the non-modular design of HMaster. I didn't want to refactor the whole class in order to add this feature. As for the design, I'm open to suggestions, if you have anything more specific.
This was my original strategy. It doesn't work well, because master owns directly all these chores, and is picky about the order in which subsystems are initialized. Instantiation of the RegionNormalizerManager is also in an odd place, because of conditional initialization of ZKWatchers.
CatalogJanitor is a good deal simpler and conceptually makes sense being limited in scope to a Chore. It doesn't have a "switch" in ZooKeeper to manage, nor does it have a worker with a background thread. It appears there's been some time spent recently on the design of CatalogJanitor as a subsystem. Let me study it a bit and see what lessons I have to learn here.
Sure thing. Since you are both so allergic to the Optional class, I'll swap it for a null check. |
…e shutdown NPE ADDENDUM)
ndimiduk
force-pushed
the
24628-normalizer-master-chore-shutdown-npe-addendum
branch
from
e4075b7
to
685ebb3
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Oct 15, 2020
|
Yes, the initialization order of HMaster is just a mess, too many dependencies... I like that you want to keep align with the current design of how to schedule chores. It is good to keep the code in one style so later developpers will not be confused. What I said is just suggestion, I'm OK with the current implementation. Let's see if this could fix the flaky tests. |
Apache9
pushed a commit
that referenced
this pull request
Oct 15, 2020
…e shutdown NPE ADDENDUM) (#2540) Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
ndimiduk
deleted the
24628-normalizer-master-chore-shutdown-npe-addendum
branch
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…e shutdown NPE ADDENDUM) (apache#2540) Signed-off-by: Michael Stack <stack@apache.org> Signed-off-by: Duo Zhang <zhangduo@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.
I think this variable is null for backup masters.
initializeZKBasedSystemTrackersonly called when a master transitions to active. I don't know why it impacts this test though, as an NPE in the backup master thread should cause the thread to halt.