HBASE-23205 Correctly update the position of WALs currently being replicated #749
Conversation
|
💔 -1 overall
This message was automatically generated. |
JeongDaeKim
force-pushed
the
HBASE-23205
branch
from
b3e9a4e to
a8244d2
Compare
|
💔 -1 overall
This message was automatically generated. |
|
I made a typo when i fixed checkstyle warnings 😭 (a8244d2#diff-7d551f2261f4c83aec8a97b7d04427e2R137) Let me fix it. |
|
💔 -1 overall
This message was automatically generated. |
JeongDaeKim
force-pushed
the
HBASE-23205
branch
from
df8efb2 to
114aa1b
Compare
|
added a minor fix in test code and rebased. |
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Thanks for the analysis. Had done a first round of reading through, but the PR seems a bit large to grasp it all in one go, hence some of the question in my comments.
It would be nice to keep changes to a minimal, adding only modifications really needed to fix the problem. For example, there are few variable/method renaming, moving to different class, just for personal/cosmetic preferences, together with additional unrelated fixes, such as the mentioned metric one (if that's not needed here, please open a separate jira to it).
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSmallTests.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
Outdated
Show resolved
Hide resolved
| break; | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Where is the log position getting updated now if the current edit is not targeted to replication?
There was a problem hiding this comment.
I tried not to update log position every single filtered entry. because I found a lot of updates (setData) happened in zookeeper tx logs, even though all entries were filtered.
log position will be updated in these case.
- limits(quota, size, count) reached, or a batch has entries when eof reached.
- wal rolled
- when reader read all wals in recovery queue.
I think updating log position is required for 1) cleanup old logs, 2) replication queue recovery.
for 1) it would be enough to update log position only when log rolled.
for 2) the log position should be updated to the position of the last replicated entry.
in any case, we don't need to update log position aggressively for filtered entries. entries would be filtered again for recovery case.
There was a problem hiding this comment.
So if a filtered edit came, any sub-sequent non-filterable one would need to wait for a log roll? That could take too long for some use cases.
There was a problem hiding this comment.
any sub-sequent non-filterable one would need to wait for a log roll? That could take too long for some use cases.
If some use cases means "no mutations come for a long time, but a batch has entries", this case is the one of the 1) case i mentioned a batch has entries when eof reached. reader would reach the eof, and log position would be updated.
In addition, even while testing this issue with heavy writes, I observed the reader frequently reached EOF.
There was a problem hiding this comment.
If some use cases means "no mutations come for a long time, but a batch has entries"
What if the whole WAL section read got no entries for replication? In this case, batch would be empty, so ReplicationSourceManager.logPositionAndCleanOldLogs does not ever get called (at least, I guess, until the log is rolled).
There was a problem hiding this comment.
I think the answer to my question above is in the resetStream() that gets called at the end of the second while loop, which will update lastReadPosition variable that is now used for reading here.
There was a problem hiding this comment.
What if the whole WAL section read got no entries for replication? In this case, batch would be empty, so ReplicationSourceManager.logPositionAndCleanOldLogs does not ever get called (at least, I guess, until the log is rolled).
Yes, In that case, the position will be updated when log rolled. That is my intention. #749 (comment)
I think the answer to my question above is in the resetStream() that gets called at the end of the second while loop, which will update lastReadPosition variable that is now used for reading here.
Oh? Then, I think i didn't understand what your question is. 🤣
There was a problem hiding this comment.
I guess this is fine for the replication progress problem. One additional issue, though, is regarding monitoring. IIRC, DumpReplicationQueues relies on replication info available at ZK, so now it may not show an accurate position for the log queue. We may need to expose ReplicationSourceWALReaderThread.lastReadPosition via getter method for monitoring purposes.
.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Outdated
Show resolved
Hide resolved
.../java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceWALReaderThread.java
Show resolved
Hide resolved
| @@ -378,37 +380,35 @@ public void testReplicationSourceWALReaderThread() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testReplicationSourceUpdatesLogPositionOnFilteredEntries() throws Exception { | |||
| public void testReplicationSourceWALReaderThreadRecoveredQueue() throws Exception { | |||
There was a problem hiding this comment.
Why this test has been changed from it's original purpose of checking for upating wal position when no edits targeted to replication are read? Also the name does not seem accurate, it does not seem to create a recovered queue scenario.
There was a problem hiding this comment.
I removed testReplicationSourceUpdatesLogPositionOnFilteredEntries, because the behavior of the reader is changed. (no updates every filtered entry).
And, i made recovered queue scenario by setting queue info as recovered queue. getQueueInfo("1-1")
https://github.com/apache/hbase/pull/749/files/114aa1b1a7b9919c5429fadcb74079cd08629513#diff-05e8e2a626166f52e5737f8bcdc49e39R401
If it is not well recognized as intended, let me add comments or getRecoveredQueueInfo()?
There was a problem hiding this comment.
I changed the method name to getRecoveredQueueInfo()
There was a problem hiding this comment.
Actually yeah, this is indeed simulating recovered queue when creating it as recovered. I think this test is fine.
Signed-off-by: Wellington Chevreuil <[email protected]>
|
Thanks for the review!
I see. I'll make my changes smaller to remain only necessary ones.
Let me file a new jira then 👍 |
|
🎊 +1 overall
This message was automatically generated. |
…CallableWithReplicas (apache#780) Signed-off-by: Sean Busbey <[email protected]>
…alue every time [Take2] (apache#748) * HBASE-23185 Fix test failure by HBASE-23185 changes * HBASE-23185 Fix high cpu usage because getTable()#put() gets config value every time This reverts commit db2ce23.
Signed-off-by: Andrew Purtell <[email protected]>
…che#789) Signed-off-by: Sean Busbey <[email protected]> Signed-off-by: Guangxu Cheng <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
…ould be at INFO Signed-off-by: Jan Hentschel <[email protected]>
|
The failed test seems not related to this pr. (It failed after a commit just adding a new line), and I can't reproduce it in my local repo. I think PR is ready to get review. please have a look. @wchevreuil |
|
No further comments on this PR? If any lacks of description or something for code reviews, please let me know. If not, I just want this PR to be merged, and backported to 1.4. @wchevreuil Do you still have something to be changed in this PR? |
|
Hi @JeongDaeKim , apologies for the delay. I think the solution is good, but since this is changing considerably how we track log reading position, am just taking a conservative approach. I would like to do a bit of testing. Please give me until end of this week to approve it, or suggest changes. |
There was a problem hiding this comment.
Thanks for your patience, @JeongDaeKim ! I think I finally got a better understanding of the chages logic, along with the new and modified tests. I had put on some additional comments within the code, but some additional thoughts:
- We might want to expose
ReplicationSourceWALReaderThread.lastReadPosition, in order to eventually have an accurate monitoring info. Currently we have _DumpReplicationQueues` which just reads info from ZK. Maybe we should print a warning there that reported log position may not be accurate. - Can we add a 3rd test on TestWalEntryStream that adds few filterable entries, then adds a non filterable one, and checks if this non filterable comes in from the batch?
| @@ -378,37 +380,35 @@ public void testReplicationSourceWALReaderThread() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testReplicationSourceUpdatesLogPositionOnFilteredEntries() throws Exception { | |||
| public void testReplicationSourceWALReaderThreadRecoveredQueue() throws Exception { | |||
There was a problem hiding this comment.
Actually yeah, this is indeed simulating recovered queue when creating it as recovered. I think this test is fine.
| break; | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I guess this is fine for the replication progress problem. One additional issue, though, is regarding monitoring. IIRC, DumpReplicationQueues relies on replication info available at ZK, so now it may not show an accurate position for the log queue. We may need to expose ReplicationSourceWALReaderThread.lastReadPosition via getter method for monitoring purposes.
| // reader won't put any batch, even if EOF reached. | ||
| ExecutorService executor = Executors.newSingleThreadExecutor(); | ||
| Future<WALEntryBatch> future = executor.submit(new Callable<WALEntryBatch>() { | ||
| @Override | ||
| public WALEntryBatch call() throws Exception { | ||
| return reader.take(); | ||
| } | ||
| }); | ||
| Thread.sleep(2000); | ||
| assertFalse(future.isDone()); |
There was a problem hiding this comment.
Just a heads up here: we may simplify this part if we decide to make ReplicationSourceWALReaderThread.lastReadPosition exposed via getter method.
) - switch to nexus-staging-maven-plugin for asf-release - cleaned up some tabs in the root pom (differs from master because there are no release scripts here.) Signed-off-by: stack <[email protected]> (cherry picked from commit 97e0107)
…ction disabled in branch-1 (apache#899) Signed-off-by: Balazs Meszaros <[email protected]> Signed-off-by Anoop Sam John <[email protected]>
…File is a reference file Signed-off-by: Lijin Bin <[email protected]>
… to a capacity rule (apache#894) Signed-off-by Wellington Chevreuil <[email protected]>
We have this nice description in the java doc on ITBLL but it's unformatted and thus illegible. Add some formatting so that it can be read by humans. Signed-off-by: Jan Hentschel <[email protected]> Signed-off-by: Josh Elser <[email protected]>
I think "reported log position" from DumpReplicationQueues could not be current read position before this PR (no updates during making a batch).
I see, added a new test |
…pache#896) Differs from original by removing Capacity Unit examples since that feature isn't on this branch. (cherry picked from commit a553b78) (cherry picked from commit 2a1efe0)
|
No luck with the pre-commit. I tried a rebase, but still the job fails while starting. Ain't sure if it's something specific to the PR commits. @JeongDaeKim , would u mind squash these commits on one of you local branches, then open a new PR for branch-1 with this? Please ping me once you open the new PR. |
|
Merged PR 944, so am closing this one. Thanks for the contribution and patience, @JeongDaeKim ! |
https://issues.apache.org/jira/browse/HBASE-23205
I fixed a failed test which is not related with this PR.TestReplicationSmallTests.testEmptyWALRecovery,and a minor bug which is updating replication buffer size wrongly by decreasing total buffer size with the size of bulk loaded files.
I removed the changes above and made a separate jira : https://issues.apache.org/jira/browse/HBASE-23254