Uh oh!
There was an error while loading. Please reload this page.
HBASE-24665 MultiWAL : Avoid rolling of ALL WALs when one of the WAL needs a roll - #2021
Merged
Merged
Conversation
Apache-HBase
commented
Jul 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 6, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 6, 2020
🎊 +1 overall
This message was automatically generated. |
| for (Iterator<Entry<WAL, Boolean>> iter = walNeedsRoll.entrySet().iterator(); iter | ||
| .hasNext();) { | ||
| Entry<WAL, Boolean> entry = iter.next(); | ||
| if (!periodic && !entry.getValue()) { |
Contributor
There was a problem hiding this comment.
The actual fix is this alone right?
| long now = System.currentTimeMillis(); | ||
| checkLowReplication(now); | ||
| periodic = (now - this.lastRollTime) > this.rollPeriod; | ||
| boolean periodic = (now - this.lastRollTime) > this.rollPeriod; |
Contributor
There was a problem hiding this comment.
Not on this patch directly.
We have the periodic WAL roll.. Every WAL should get rolled as per this period. When one WAL gets rolled, we will change the lastRollTime and so it can happen that some WALs will ever get rolled!
This issue will be visible now as we will selectively roll WAL files after this patch. Actually we need to track the lastRollTime per WAL instance.
ContributorAuthor
There was a problem hiding this comment.
It's like this, I ignored it. Thanks for reminding
Apache-HBase
commented
Jul 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 10, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 10, 2020
🎊 +1 overall
This message was automatically generated. |
| this.lastRollTime = System.currentTimeMillis(); | ||
| } | ||
| public boolean isRequestRoll() { |
Contributor
There was a problem hiding this comment.
isRollRequested() can be the better name?
| @@ -66,6 +67,10 @@ protected void scheduleFlush(String encodedRegionName, List<byte[]> families) { | |||
| @VisibleForTesting | |||
| Map<WAL, Boolean> getWalNeedsRoll() { | |||
Contributor
There was a problem hiding this comment.
This is exposed for test cases only. This is a private class.. So its ok to change the return type. The new RollController gives clear idea whether a wal instance needs roll because of periodic roll or being explicitly asked for. So that is better.
A return type of Map<WAL, RollController>
Any way then we dont need synchronized block. Else, if we have to do as what is being done below in patch, we would need synchronized block
| protected static final String WAL_ROLL_PERIOD_KEY = "hbase.regionserver.logroll.period"; | ||
| protected final ConcurrentMap<WAL, Boolean> walNeedsRoll = new ConcurrentHashMap<>(); | ||
| protected final ConcurrentMap<WAL, RollController> walNeedsRoll = new ConcurrentHashMap<>(); |
Contributor
There was a problem hiding this comment.
We add all WAL instances into this once it is created. We can just call it wals?
| return isRequestRoll; | ||
| } | ||
| boolean isPeriodRoll(long now) { |
Contributor
There was a problem hiding this comment.
A better name ? needsPeriodicRoll ?
| this.isRequestRoll = true; | ||
| } | ||
| void finishRoll() { |
Contributor
There was a problem hiding this comment.
This name is bit confusing. This is not called once roll is finished. We can just call this resetStatus()? Give proper comment that this resets rollReq status as well as lastRollTime. We can pass the ts as param 'lastRollTime' so that this is clear.
| iter.hasNext();) { | ||
| Entry<WAL, RollController> entry = iter.next(); | ||
| RollController controller = entry.getValue(); | ||
| if (!controller.isRequestRoll && !controller.isPeriodRoll(now)) { |
Contributor
There was a problem hiding this comment.
We can have a single method in RollController which says this WAL needs roll? RollController#needsRoll().
| if (periodic) { | ||
| if (walNeedsRoll.values().stream().anyMatch(rc -> rc.isPeriodRoll(now))) { | ||
| // Time for periodic roll, fall through | ||
| LOG.debug("WAL roll period {} ms elapsed", this.rollPeriod); |
Contributor
There was a problem hiding this comment.
This log is not much value added now as we dont say for which log(s) need this periodic roll. We can clearly say WAL roll period {} elapsed for one of the WAL.
Below we can make sure we log which wal(s) are getting rolled for what purpose
| if (walNeedsRoll.values().stream().anyMatch(Boolean::booleanValue)) { | ||
| if (walNeedsRoll.values().stream().anyMatch(rc -> rc.isRequestRoll)) { | ||
| // WAL roll requested, fall through | ||
| LOG.debug("WAL roll requested"); |
| WAL wal = entry.getKey(); | ||
| // reset the flag in front to avoid missing roll request before we return from rollWriter. | ||
| walNeedsRoll.put(wal, Boolean.FALSE); | ||
| controller.finishRoll(); |
Contributor
There was a problem hiding this comment.
Ideally we need this call also within a synchronized block. This is an existing issue.
ContributorAuthor
There was a problem hiding this comment.
Thank you very much for your tips
| this.isRequestRoll = true; | ||
| } | ||
| public synchronized Map<byte[], List<byte[]>> rollWal(long lastRollTime) throws IOException { |
Contributor
There was a problem hiding this comment.
This is overall coming good now. Much more clean than what we had before. Thanks
One issue here though. We have rollWal() and requestRoll() under same lock now.. That is not good. The requestRoll should not wait for ongoing roll to be finished. The request call is in hot path.
| for (Entry<WAL, RollController> entry : wals.entrySet()) { | ||
| WAL wal = entry.getKey(); | ||
| boolean needRollAlready = entry.getValue(); | ||
| boolean needRollAlready = entry.getValue().isRollRequested(); |
Contributor
There was a problem hiding this comment.
Previously, when we are going for a roll because of periodic roll, then also we used to keep the value in Map as TRUE right? Means here we checked that also. Because of any reason, the WAL is marked for roll, dont do this check. So here that is missing now.
Apache-HBase
commented
Jul 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 13, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 14, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 14, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 14, 2020
💔 -1 overall
This message was automatically generated. |
| } | ||
| public void requestRoll() { | ||
| this.rollRequestCounter.incrementAndGet(); |
Contributor
There was a problem hiding this comment.
Am not sure whether this way of count based thing also correct. Like say 2 times req roll happened before the actual rolling thread picks up this work. Effectively we need to do roll only once right. Even if 2 or more times being asked. But now as per this change it will try do roll again and again.
So a boolean based thing is enough
What we can do is make the requestRoll being synchronized and within rollWal have a synchronized block where you can reset it ad then go ahead with actual roll call.
Or we just need an AtomicBoolean only?
Apache-HBase
commented
Jul 20, 2020
🎊 +1 overall
This message was automatically generated. |
| public Map<byte[], List<byte[]>> rollWal(long now) throws IOException { | ||
| this.lastRollTime = now; | ||
| Map<byte[], List<byte[]>> regionsToFlush = wal.rollWriter(true); | ||
| this.rollRequest.set(false); |
Contributor
There was a problem hiding this comment.
Hey this is where we need to reset it even before we do actual roll work.. We were doing that. We had a comment also.
// reset the flag in front to avoid missing roll request before we return from rollWriter.
ContributorAuthor
There was a problem hiding this comment.
thank you very much! I also noticed this, but in order to avoid repeated roll, so fix to that. if use boolean, roll at most once, the impact can be ignored. Thanks for review
Contributor
There was a problem hiding this comment.
Sorry did not get. How AtomicBoolean is different compared to boolean wrt repeated roll possibility. In the past we used to keep the Boolean state in a Map. Now in this object.
ContributorAuthor
There was a problem hiding this comment.
after reset flag, before roll is complete, if have a roll request, that will also cause an extra roll.
Contributor
There was a problem hiding this comment.
Yes. That is happening now also. We intentionally reset the boolean when we start roll itself.
// reset the flag in front to avoid missing roll request before we return from rollWriter.
So there is no change for that right now.
Apache-HBase
commented
Jul 20, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 20, 2020
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
left a comment
Contributor
There was a problem hiding this comment.
Last set of comments.
Overall looks good. +1
| walNeedsRoll.put(wal, Boolean.FALSE); | ||
| Map<byte[], List<byte[]>> regionsToFlush = null; | ||
| RollController controller = entry.getValue(); | ||
| boolean isRequestRoll; |
Contributor
There was a problem hiding this comment.
Seems this boolean is not needed now. Can u pls delete
| * @return true if all WAL roll finished | ||
| */ | ||
| public boolean walRollFinished() { | ||
| return walNeedsRoll.values().stream().allMatch(needRoll -> !needRoll) && isWaiting(); |
Contributor
There was a problem hiding this comment.
Checking its usage, I think this API impl is already buggy. This just checks the status of the boolean. Once we start a roll on a WAL, we reset the boolean (Even before this patch). So it is not clearly telling anything abt the roll status. This can return true even while an active wal roll is going on. We can keep it as an another jira and fix (if required).. Just add some TODO comments here.
We might need another boolean in Controller which clearly tracks whether we are ongoing a roll. So this really need to check that status as well as a requested roll status.
| */ | ||
| public boolean walRollFinished() { | ||
| return walNeedsRoll.values().stream().allMatch(needRoll -> !needRoll) && isWaiting(); | ||
| return wals.values().stream().noneMatch(RollController::isRollRequested) && isWaiting(); |
Contributor
There was a problem hiding this comment.
Here rather than isRollRequested, we need needsRoll?
| wal.init(); | ||
| wals.put(wal, wal.getCurrentFileName()); | ||
| ROLLER.addWAL(wal); | ||
| Thread.sleep(3000); |
Contributor
There was a problem hiding this comment.
We need a 3 sec sleep here? !
Contributor
There was a problem hiding this comment.
We want 3 WALs will different periodic roll time period? Why we need? If so, can we limit the time to be 1 sec or lesser?
| walEntry.setValue(walEntry.getKey().getCurrentFileName()); | ||
| while (it.hasNext()) { | ||
| walEntry = it.next(); | ||
| assertEquals(walEntry.getValue(), walEntry.getKey().getCurrentFileName()); |
Apache-HBase
commented
Jul 21, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2020
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2020
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Jul 22, 2020
| protected static final String WAL_ROLL_PERIOD_KEY = "hbase.regionserver.logroll.period"; | ||
| protected final ConcurrentMap<WAL, Boolean> walNeedsRoll = new ConcurrentHashMap<>(); | ||
| protected final ConcurrentMap<WAL, RollController> wals = new ConcurrentHashMap<>(); |
ContributorAuthor
There was a problem hiding this comment.
rollWals? more suitable？
Contributor
There was a problem hiding this comment.
This will have all wal entries once the wal instance is created. Not just when it needs a roll. So ya these were wals which needed roll at some point. So wals name also fine IMO.
| wal.registerWALActionsListener(new WALActionsListener() { | ||
| @Override | ||
| public void logRollRequested(WALActionsListener.RollRequestReason reason) { | ||
| // TODO logs will contend with each other here, replace with e.g. DelayedQueue |
Contributor
There was a problem hiding this comment.
Is this talking about what this PR is trying to do?
ContributorAuthor
There was a problem hiding this comment.
No, the todo is pre-existing.
ContributorAuthor
There was a problem hiding this comment.
@ramkrish86 the purpose of this PR is to make each wal separate roll when using multiwal. thanks review.
| // Time for periodic roll, fall through | ||
| LOG.debug("WAL {} roll period {} ms elapsed", wal, this.rollPeriod); | ||
| } else { | ||
| continue; |
Contributor
There was a problem hiding this comment.
This is they key. We ensure we only roll if one of the condition is met (if either size reach caused a log roll ) or the time elapsed. That is also tracked per wal.
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left one comment, overall seems good.
| // TODO logs will contend with each other here, replace with e.g. DelayedQueue | ||
| synchronized (AbstractWALRoller.this) { | ||
| walNeedsRoll.put(wal, Boolean.TRUE); | ||
| RollController controller = wals.computeIfAbsent(wal, rc -> new RollController(wal)); |
Contributor
There was a problem hiding this comment.
We have already done wals.putIfAbsent(wal, new RollController(wal)) above.
Hence, wals.computeIfAbsent() is needed here? Should we not directly get the value with RollController controller = wals.get(wal) and expect non-null object?
Contributor
There was a problem hiding this comment.
Good Q. In fact I also thought when reviewed this. Ideally speaking we should get the addWAL call 1st which will add the instance to the Map. When we get call here the wal should be in the map already. But if u see the cur impl, there is no such contract enforcing. It just add the WAL with True value. So believe while making patch, @WenFeiYi went with similar lines.
We can consider this.. Need to see any chance we get a rollReq before adding.. While RS start, we do some rollReq on WALs.. This introduced some bug in the past. We need to see that closely.. If we can confirm that we can add that contract enforcing and so what u suggested. I would say add a TODO here and raise another issue. This went through multiple cycles of changes. :-) U ok Viraj?
ContributorAuthor
There was a problem hiding this comment.
yes, this is my idea, if there is a change, the impact will be greater, it will be far away from the purpose of the issue.
Contributor
There was a problem hiding this comment.
Sure, this is critical code anyways, we can live with bit extra atomic calls on Concurrent Map. Sounds good, TODO should make this better :)
Nice work @WenFeiYi .
virajjasani
approved these changes
Jul 24, 2020
| // TODO logs will contend with each other here, replace with e.g. DelayedQueue | ||
| synchronized (AbstractWALRoller.this) { | ||
| walNeedsRoll.put(wal, Boolean.TRUE); | ||
| RollController controller = wals.computeIfAbsent(wal, rc -> new RollController(wal)); |
Contributor
There was a problem hiding this comment.
Sure, this is critical code anyways, we can live with bit extra atomic calls on Concurrent Map. Sounds good, TODO should make this better :)
Nice work @WenFeiYi .
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
No description provided.