Uh oh!
There was an error while loading. Please reload this page.
HBASE-23205 Correctly update the position of WALs currently being replicated - #980
Conversation
Apache-HBase
commented
Jan 3, 2020
💔 -1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
Looking good. Need to come back to this. Substantial change.
| @VisibleForTesting | ||
| public Path getLastLoggedPath() { | ||
| for (ReplicationSourceShipperThread worker : workerThreads.values()) { | ||
| return worker.getLastLoggedPath(); |
There was a problem hiding this comment.
Are the workerThreads sorted so we get last logged first?
There was a problem hiding this comment.
Not sorted, It's only for testing in non-multi wal environment, as same as ReplicationSource.getCurrentPath().
wchevreuil
left a comment
There was a problem hiding this comment.
Looks in line with the changes of PR #944 (merged into branch-1). So LGTM, but let's wait for @saintstack review.
saintstack
left a comment
There was a problem hiding this comment.
I don't know this code well but what is here looks good to me. A few questions in the below.
| long position = worker.getCurrentPosition(); | ||
| Path currentPath = worker.getCurrentPath(); | ||
| long position = worker.getLastLoggedPosition(); | ||
| Path currentPath = worker.getLastLoggedPath(); |
There was a problem hiding this comment.
This is not for test, right? We iterate the workers twice -- once to get path and then again to get position. Don't want to return a Pair with Path and offset so just iterate once?
There was a problem hiding this comment.
Hmm... reading later in the patch, I see need to have them distinct. For sure we'll get the right offset for the selected path?
There was a problem hiding this comment.
Yes, not for test and not the same one. each thread have it's own path and position. no need to iterate
| // set "ageOfLastShippedOp" to <now> to indicate that we're current | ||
| metrics.setAgeOfLastShippedOp(EnvironmentEdgeManager.currentTime(), walGroupId); | ||
| } | ||
| updateLogPosition(lastReadPosition); |
There was a problem hiding this comment.
Will we be doing a bunch of spinning updating same value over and over w/o these checks in place that look to see if position has changed? Will we be burning CPU to no end?
There was a problem hiding this comment.
It won't be updated with the same value repeatably. entryReader will put a batch only when read position is changed.
| } | ||
| private void updateLogPosition(long lastReadPosition) { | ||
| manager.setPendingShipment(false); |
There was a problem hiding this comment.
Its ok to remove this setting?
There was a problem hiding this comment.
We don't do pending flag anymore?
There was a problem hiding this comment.
Yes, we don't need the flag anymore. it was required because not only shipper but also reader thread can update log position, but it had some issues about concurrency and high update frequency. With this PR, when wal rolled or log position changed, reader will put a batch for shipper to update log position.
JeongDaeKim
commented
Jan 9, 2020
Thanks all for approvals! Could you merge this, if no more comments or questions? |
wchevreuil
commented
Jan 9, 2020
Thanks again for all the work here, @JeongDaeKim ! |
…licated (apache#980) Signed-off-by: stack <stack@apache.org> Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit 879b7ea)
https://issues.apache.org/jira/browse/HBASE-23205
related PR : #944