You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/01/03 09:35:06 UTC
[GitHub] [hbase] JeongDaeKim opened a new pull request #980: HBASE-23205
Correctly update the position of WALs currently being replicated
JeongDaeKim opened a new pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980
https://issues.apache.org/jira/browse/HBASE-23205
related PR : #944
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] wchevreuil merged pull request #980: HBASE-23205 Correctly
update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
wchevreuil merged pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] JeongDaeKim commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
JeongDaeKim commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363610534
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -727,8 +740,7 @@ protected void shipEdits(WALEntryBatch entryBatch) {
}
private void updateLogPosition(long lastReadPosition) {
- manager.setPendingShipment(false);
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r362893413
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -436,14 +437,30 @@ public String getPeerClusterId() {
}
@Override
+ @VisibleForTesting
public Path getCurrentPath() {
- // only for testing
for (ReplicationSourceShipperThread worker : workerThreads.values()) {
if (worker.getCurrentPath() != null) return worker.getCurrentPath();
}
return null;
}
+ @VisibleForTesting
+ public Path getLastLoggedPath() {
+ for (ReplicationSourceShipperThread worker : workerThreads.values()) {
+ return worker.getLastLoggedPath();
Review comment:
Are the workerThreads sorted so we get last logged first?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363597077
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -635,15 +650,13 @@ private void checkBandwidthChangeAndResetThrottler() {
protected void shipEdits(WALEntryBatch entryBatch) {
List<Entry> entries = entryBatch.getWalEntries();
long lastReadPosition = entryBatch.getLastWalPosition();
- currentPath = entryBatch.getLastWalPath();
+ lastLoggedPath = entryBatch.getLastWalPath();
int sleepMultiplier = 0;
if (entries.isEmpty()) {
- if (lastLoggedPosition != lastReadPosition) {
- updateLogPosition(lastReadPosition);
- // if there was nothing to ship and it's not an error
- // set "ageOfLastShippedOp" to <now> to indicate that we're current
- metrics.setAgeOfLastShippedOp(EnvironmentEdgeManager.currentTime(), walGroupId);
- }
+ updateLogPosition(lastReadPosition);
Review 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] JeongDaeKim commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
JeongDaeKim commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363610492
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -635,15 +650,13 @@ private void checkBandwidthChangeAndResetThrottler() {
protected void shipEdits(WALEntryBatch entryBatch) {
List<Entry> entries = entryBatch.getWalEntries();
long lastReadPosition = entryBatch.getLastWalPosition();
- currentPath = entryBatch.getLastWalPath();
+ lastLoggedPath = entryBatch.getLastWalPath();
int sleepMultiplier = 0;
if (entries.isEmpty()) {
- if (lastLoggedPosition != lastReadPosition) {
- updateLogPosition(lastReadPosition);
- // if there was nothing to ship and it's not an error
- // set "ageOfLastShippedOp" to <now> to indicate that we're current
- metrics.setAgeOfLastShippedOp(EnvironmentEdgeManager.currentTime(), walGroupId);
- }
+ updateLogPosition(lastReadPosition);
Review comment:
It won't be updated with the same value repeatably. entryReader will put a batch only when read position is changed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363597229
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -727,8 +740,7 @@ protected void shipEdits(WALEntryBatch entryBatch) {
}
private void updateLogPosition(long lastReadPosition) {
- manager.setPendingShipment(false);
Review comment:
We don't do pending flag anymore?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363597156
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -727,8 +740,7 @@ protected void shipEdits(WALEntryBatch entryBatch) {
}
private void updateLogPosition(long lastReadPosition) {
- manager.setPendingShipment(false);
Review comment:
Its ok to remove this setting?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] JeongDaeKim commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
JeongDaeKim commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363610353
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -478,8 +495,8 @@ public String getStats() {
for (Map.Entry<String, ReplicationSourceShipperThread> entry : workerThreads.entrySet()) {
String walGroupId = entry.getKey();
ReplicationSourceShipperThread worker = entry.getValue();
- long position = worker.getCurrentPosition();
- Path currentPath = worker.getCurrentPath();
+ long position = worker.getLastLoggedPosition();
+ Path currentPath = worker.getLastLoggedPath();
Review comment:
Yes, not for test and not the same one. each thread have it's own path and position. no need to iterate
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363591365
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -478,8 +495,8 @@ public String getStats() {
for (Map.Entry<String, ReplicationSourceShipperThread> entry : workerThreads.entrySet()) {
String walGroupId = entry.getKey();
ReplicationSourceShipperThread worker = entry.getValue();
- long position = worker.getCurrentPosition();
- Path currentPath = worker.getCurrentPath();
+ long position = worker.getLastLoggedPosition();
+ Path currentPath = worker.getLastLoggedPath();
Review 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] Apache-HBase commented on issue #980: HBASE-23205 Correctly
update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on issue #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#issuecomment-570556914
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 41s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
||| _ branch-1.4 Compile Tests _ |
| +1 :green_heart: | mvninstall | 8m 7s | branch-1.4 passed |
| +1 :green_heart: | compile | 0m 39s | branch-1.4 passed with JDK v1.8.0_232 |
| +1 :green_heart: | compile | 0m 45s | branch-1.4 passed with JDK v1.7.0_242 |
| +1 :green_heart: | checkstyle | 1m 35s | branch-1.4 passed |
| +1 :green_heart: | shadedjars | 2m 47s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | branch-1.4 passed with JDK v1.8.0_232 |
| +1 :green_heart: | javadoc | 0m 40s | branch-1.4 passed with JDK v1.7.0_242 |
| +0 :ok: | spotbugs | 2m 49s | Used deprecated FindBugs config; considering switching to SpotBugs. |
| +1 :green_heart: | findbugs | 2m 47s | branch-1.4 passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 56s | the patch passed |
| +1 :green_heart: | compile | 0m 39s | the patch passed with JDK v1.8.0_232 |
| +1 :green_heart: | javac | 0m 39s | the patch passed |
| +1 :green_heart: | compile | 0m 47s | the patch passed with JDK v1.7.0_242 |
| +1 :green_heart: | javac | 0m 47s | the patch passed |
| -1 :x: | checkstyle | 1m 34s | hbase-server: The patch generated 1 new + 25 unchanged - 10 fixed = 26 total (was 35) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | shadedjars | 2m 39s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | hadoopcheck | 2m 30s | Patch does not cause any errors with Hadoop 2.7.7. |
| +1 :green_heart: | javadoc | 0m 30s | the patch passed with JDK v1.8.0_232 |
| +1 :green_heart: | javadoc | 0m 40s | the patch passed with JDK v1.7.0_242 |
| +1 :green_heart: | findbugs | 2m 51s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 118m 36s | hbase-server in the patch passed. |
| +1 :green_heart: | asflicense | 0m 34s | The patch does not generate ASF License warnings. |
| | | 155m 34s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.5 Server=19.03.5 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-980/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/980 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs findbugs shadedjars hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 7a7589a46ec7 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /home/jenkins/jenkins-slave/workspace/HBase-PreCommit-GitHub-PR_PR-980/out/precommit/personality/provided.sh |
| git revision | branch-1.4 / aaa2266 |
| Default Java | 1.7.0_242 |
| Multi-JDK versions | /usr/lib/jvm/zulu-8-amd64:1.8.0_232 /usr/lib/jvm/zulu-7-amd64:1.7.0_242 |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-980/1/artifact/out/diff-checkstyle-hbase-server.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-980/1/testReport/ |
| Max. process+thread count | 4346 (vs. ulimit of 10000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-980/1/console |
| versions | git=1.9.1 maven=3.0.5 findbugs=3.0.1 |
| Powered by | Apache Yetus 0.11.1 https://yetus.apache.org |
This message was automatically generated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] wchevreuil commented on issue #980: HBASE-23205 Correctly
update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on issue #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#issuecomment-572488574
Thanks again for all the work here, @JeongDaeKim !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] saintstack commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
saintstack commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363596599
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -478,8 +495,8 @@ public String getStats() {
for (Map.Entry<String, ReplicationSourceShipperThread> entry : workerThreads.entrySet()) {
String walGroupId = entry.getKey();
ReplicationSourceShipperThread worker = entry.getValue();
- long position = worker.getCurrentPosition();
- Path currentPath = worker.getCurrentPath();
+ long position = worker.getLastLoggedPosition();
+ Path currentPath = worker.getLastLoggedPath();
Review 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] JeongDaeKim commented on issue #980: HBASE-23205 Correctly
update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
JeongDaeKim commented on issue #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#issuecomment-572430147
Thanks all for approvals! Could you merge this, if no more comments or questions?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [hbase] JeongDaeKim commented on a change in pull request #980:
HBASE-23205 Correctly update the position of WALs currently being replicated
Posted by GitBox <gi...@apache.org>.
JeongDaeKim commented on a change in pull request #980: HBASE-23205 Correctly update the position of WALs currently being replicated
URL: https://github.com/apache/hbase/pull/980#discussion_r363195483
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
##########
@@ -436,14 +437,30 @@ public String getPeerClusterId() {
}
@Override
+ @VisibleForTesting
public Path getCurrentPath() {
- // only for testing
for (ReplicationSourceShipperThread worker : workerThreads.values()) {
if (worker.getCurrentPath() != null) return worker.getCurrentPath();
}
return null;
}
+ @VisibleForTesting
+ public Path getLastLoggedPath() {
+ for (ReplicationSourceShipperThread worker : workerThreads.values()) {
+ return worker.getLastLoggedPath();
Review comment:
Not sorted, It's only for testing in non-multi wal environment, as same as ReplicationSource.getCurrentPath().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services