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 2022/01/26 09:19:26 UTC
[GitHub] [hbase] BukrosSzabolcs opened a new pull request #4066: HBASE-26707: Reduce number of renames during bulkload
BukrosSzabolcs opened a new pull request #4066:
URL: https://github.com/apache/hbase/pull/4066
move files directly to the store dir when requireWritingToTmpDirFirst is
false
fix failedBulkLoad to work on second call
change existing tests to run without tmp folder too
add tests for SecureBulkLoadListener
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r807020934
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
Ah, yeah, then we would need to rename from the custom stage to the actual dir anyways. I think we can leave it this way.
For S3 with SFT, I think the best practice would be to pass the copy option, for consistency. Shouldn't have much difference, as s3 renames are basically copies.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1022153026
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 26s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 5s | master passed |
| +1 :green_heart: | compile | 1m 53s | master passed |
| +1 :green_heart: | shadedjars | 8m 49s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 6s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 50s | the patch passed |
| +1 :green_heart: | compile | 1m 54s | the patch passed |
| +1 :green_heart: | javac | 1m 54s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 54s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 5s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 149m 2s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 17s | hbase-it in the patch passed. |
| | | 187m 14s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux eaf6d5b6025b 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c33fa3ab78 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 4054 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r807020934
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
Ah, yeah, then we would need to rename from the custom stage to the actual dir anyways. I think we can leave it this way.
For S3 with SFT, I think the best practice would be to pass the copy option, for consistency. Shouldn't have much difference, as s3 renames are basically copies.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1026340301
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 7m 8s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 9s | master passed |
| +1 :green_heart: | compile | 1m 34s | master passed |
| +1 :green_heart: | shadedjars | 8m 16s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 58s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 53s | the patch passed |
| +1 :green_heart: | compile | 1m 34s | the patch passed |
| +1 :green_heart: | javac | 1m 34s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 13s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 54s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 155m 17s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 25s | hbase-it in the patch passed. |
| | | 196m 24s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 011a41761d79 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 351caa5bcc |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 4118 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1043310518
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 17s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 57s | master passed |
| +1 :green_heart: | compile | 1m 0s | master passed |
| +1 :green_heart: | shadedjars | 5m 19s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 36s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 47s | the patch passed |
| +1 :green_heart: | compile | 1m 2s | the patch passed |
| +1 :green_heart: | javac | 1m 2s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 6s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 39s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 354m 23s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 0m 50s | hbase-it in the patch passed. |
| | | 377m 46s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux a7e13d486ed2 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8dec499181 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 1667 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799601756
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -412,35 +427,37 @@ private void closeSrcFs() throws IOException {
}
@Override
- public void failedBulkLoad(final byte[] family, final String srcPath) throws IOException {
+ public void failedBulkLoad(final byte[] family, final String stagedPath) throws IOException {
try {
- Path p = new Path(srcPath);
- if (srcFs == null) {
- srcFs = FileSystem.newInstance(p.toUri(), conf);
- }
- if (!FSUtils.isSameHdfs(conf, srcFs, fs)) {
- // files are copied so no need to move them back
+ String src = origlSources.get(stagedPath);
+ if(StringUtils.isEmpty(src)){
+ LOG.debug(stagedPath + " was not moved to staging. No need to move back");
return;
}
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
- // In case of Replication for bulk load files, hfiles are not renamed by end point during
- // prepare stage, so no need of rename here again
- if (p.equals(stageP)) {
Review comment:
I do not see how could this happen. The staging folder is a temporary folder created and then cleaned up by the bulk load process. As far as I can tell it can not be the source folder for the process.
It might have been an intentional way of preventing moving back files to their original place during the second `failedBulkLoad` call, but I do not see why would any want to do that either.
Also now we have a list of files that would only contain the path if it was moved, so checking if it is still at it's original place would be redundant.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799577345
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
Review comment:
I left it like that because `origPermissions` uses String as well so a conversion would be done anyway. But now that I think about it there is no reason to keep `origPermissions` as String either. I could move both to use Path instead.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r800702571
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
True. My main concern is how safe this rename would be, if we don't have hboss in the picture. Could concurrent bulkloads be a problem in such cases? Maybe a workaround for such deployments would be to always pass a custom staging dir?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1022159054
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 6m 39s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 7s | master passed |
| +1 :green_heart: | compile | 1m 34s | master passed |
| +1 :green_heart: | shadedjars | 8m 20s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 54s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 54s | the patch passed |
| +1 :green_heart: | compile | 1m 30s | the patch passed |
| +1 :green_heart: | javac | 1m 30s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 18s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 56s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 156m 4s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 18s | hbase-it in the patch passed. |
| | | 196m 21s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux ff83098248d8 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c33fa3ab78 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 4553 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1022220137
@Apache9 , @wchevreuil, @joshelser Could you please take a look?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1042944215
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | 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. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 58s | master passed |
| +1 :green_heart: | compile | 2m 39s | master passed |
| +1 :green_heart: | checkstyle | 0m 52s | master passed |
| +1 :green_heart: | spotbugs | 1m 49s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 10s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 47s | the patch passed |
| +1 :green_heart: | compile | 2m 49s | the patch passed |
| +1 :green_heart: | javac | 2m 49s | the patch passed |
| -0 :warning: | checkstyle | 0m 48s | hbase-server: The patch generated 24 new + 180 unchanged - 0 fixed = 204 total (was 180) |
| -0 :warning: | checkstyle | 0m 13s | hbase-it: The patch generated 62 new + 0 unchanged - 0 fixed = 62 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 12m 54s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | spotbugs | 2m 22s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 16s | The patch does not generate ASF License warnings. |
| | | 37m 2s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux db24d3d70c45 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8dec499181 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-it.txt |
| Max. process+thread count | 60 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil merged pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil merged pull request #4066:
URL: https://github.com/apache/hbase/pull/4066
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1043347250
Thanks a lot @wchevreuil for your feedback and merging this!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1043265828
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 38s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 57s | master passed |
| +1 :green_heart: | compile | 0m 57s | master passed |
| +1 :green_heart: | shadedjars | 4m 57s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 42s | the patch passed |
| +1 :green_heart: | compile | 0m 57s | the patch passed |
| +1 :green_heart: | javac | 0m 57s | the patch passed |
| +1 :green_heart: | shadedjars | 4m 56s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 32s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 309m 39s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 0m 48s | hbase-it in the patch passed. |
| | | 331m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 386f4b022b74 5.4.0-1025-aws #25~18.04.1-Ubuntu SMP Fri Sep 11 12:03:04 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 8dec499181 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 1717 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r797999383
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
Review comment:
nit: make it package private with a comment saying it's for test purposes only?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
Review comment:
nit: `origSources` instead of `origlSources`?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
public SecureBulkLoadListener(FileSystem fs, String stagingDir, Configuration conf) {
this.fs = fs;
this.stagingDir = stagingDir;
this.conf = conf;
this.origPermissions = new HashMap<>();
+ this.origlSources = new HashMap<>();
}
@Override
- public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile)
- throws IOException {
+ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile,
+ String customStaging ) throws IOException {
Path p = new Path(srcPath);
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
+
+ //store customStaging for failedBulkLoad
+ String currentStaging = stagingDir;
+ if(StringUtils.isNotEmpty(customStaging)){
+ currentStaging = customStaging;
+ }
Review comment:
nit: Line #369 is redundant if line #370 is true. How about a ternary operation?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -412,35 +427,37 @@ private void closeSrcFs() throws IOException {
}
@Override
- public void failedBulkLoad(final byte[] family, final String srcPath) throws IOException {
+ public void failedBulkLoad(final byte[] family, final String stagedPath) throws IOException {
try {
- Path p = new Path(srcPath);
- if (srcFs == null) {
- srcFs = FileSystem.newInstance(p.toUri(), conf);
- }
- if (!FSUtils.isSameHdfs(conf, srcFs, fs)) {
- // files are copied so no need to move them back
+ String src = origlSources.get(stagedPath);
+ if(StringUtils.isEmpty(src)){
+ LOG.debug(stagedPath + " was not moved to staging. No need to move back");
return;
}
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
- // In case of Replication for bulk load files, hfiles are not renamed by end point during
- // prepare stage, so no need of rename here again
- if (p.equals(stageP)) {
Review comment:
We don't need this check anymore?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
One idea for FILE SFT, could we always force the `copy` option above, instead of this one that relies on rename?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
Review comment:
Could be <String,Path> and save need for path conversion again in `failedBulkLoad`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799504294
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
public SecureBulkLoadListener(FileSystem fs, String stagingDir, Configuration conf) {
this.fs = fs;
this.stagingDir = stagingDir;
this.conf = conf;
this.origPermissions = new HashMap<>();
+ this.origlSources = new HashMap<>();
}
@Override
- public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile)
- throws IOException {
+ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile,
+ String customStaging ) throws IOException {
Path p = new Path(srcPath);
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
+
+ //store customStaging for failedBulkLoad
+ String currentStaging = stagingDir;
+ if(StringUtils.isNotEmpty(customStaging)){
+ currentStaging = customStaging;
+ }
Review comment:
Nope, it's just a suggestion. The ultimate decision is yours.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799432320
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
public SecureBulkLoadListener(FileSystem fs, String stagingDir, Configuration conf) {
this.fs = fs;
this.stagingDir = stagingDir;
this.conf = conf;
this.origPermissions = new HashMap<>();
+ this.origlSources = new HashMap<>();
}
@Override
- public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile)
- throws IOException {
+ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean copyFile,
+ String customStaging ) throws IOException {
Path p = new Path(srcPath);
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
+
+ //store customStaging for failedBulkLoad
+ String currentStaging = stagingDir;
+ if(StringUtils.isNotEmpty(customStaging)){
+ currentStaging = customStaging;
+ }
Review comment:
I prefer this format, it's more descriptive. Shows that stagingDir is the default and it is only overwritten in specific circumstances.
But I can change it if you would still prefer.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1026209397
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | 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. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 19s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 8s | master passed |
| +1 :green_heart: | compile | 4m 5s | master passed |
| +1 :green_heart: | checkstyle | 1m 23s | master passed |
| +1 :green_heart: | spotbugs | 2m 50s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 57s | the patch passed |
| +1 :green_heart: | compile | 4m 5s | the patch passed |
| +1 :green_heart: | javac | 4m 5s | the patch passed |
| -0 :warning: | checkstyle | 1m 6s | hbase-server: The patch generated 24 new + 180 unchanged - 0 fixed = 204 total (was 180) |
| -0 :warning: | checkstyle | 0m 18s | hbase-it: The patch generated 62 new + 0 unchanged - 0 fixed = 62 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 24m 39s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | spotbugs | 3m 58s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 28s | The patch does not generate ASF License warnings. |
| | | 63m 24s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 34c570628424 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 351caa5bcc |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-it.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1026330058
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 20s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 47s | master passed |
| +1 :green_heart: | compile | 1m 47s | master passed |
| +1 :green_heart: | shadedjars | 8m 23s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 2s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 32s | the patch passed |
| +1 :green_heart: | compile | 1m 46s | the patch passed |
| +1 :green_heart: | javac | 1m 46s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 18s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 0s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 142m 44s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 13s | hbase-it in the patch passed. |
| | | 179m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux ec22567ebbaa 4.15.0-161-generic #169-Ubuntu SMP Fri Oct 15 13:41:54 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 351caa5bcc |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/testReport/ |
| Max. process+thread count | 4168 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799577345
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
Review comment:
I left it like that because `origPermissions` uses String as well so a conversion would be done anyway. And while I could switch both of those to use Paths, log messages will need a string too.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r800695504
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -412,35 +427,37 @@ private void closeSrcFs() throws IOException {
}
@Override
- public void failedBulkLoad(final byte[] family, final String srcPath) throws IOException {
+ public void failedBulkLoad(final byte[] family, final String stagedPath) throws IOException {
try {
- Path p = new Path(srcPath);
- if (srcFs == null) {
- srcFs = FileSystem.newInstance(p.toUri(), conf);
- }
- if (!FSUtils.isSameHdfs(conf, srcFs, fs)) {
- // files are copied so no need to move them back
+ String src = origlSources.get(stagedPath);
+ if(StringUtils.isEmpty(src)){
+ LOG.debug(stagedPath + " was not moved to staging. No need to move back");
return;
}
- Path stageP = new Path(stagingDir, new Path(Bytes.toString(family), p.getName()));
- // In case of Replication for bulk load files, hfiles are not renamed by end point during
- // prepare stage, so no need of rename here again
- if (p.equals(stageP)) {
Review comment:
Fair enough.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r801400439
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
I do not think concurrency is an issue. `SecureBulkLoadListener` keeps track of the moved files and a separate listener is created for each region in each bulkLoad. So even with parallel bulkLoads they can not touch each other's files. Using the same source folder can be an issue but it always was an issue.
I'm not sure I understand your comment about "always pass a custom staging dir". The staging dir by default is different for each bulkLoad process. I break this by introducing the "custom staging dir" which always points to the live data folder as a workaround to skip moving hfiles to an actual staging dir without loosing the existing error handling. We can't change it and decrease the number of moves at the same time.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#issuecomment-1022054672
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 26s | 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. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 6s | master passed |
| +1 :green_heart: | compile | 3m 58s | master passed |
| +1 :green_heart: | checkstyle | 1m 30s | master passed |
| +1 :green_heart: | spotbugs | 2m 44s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 54s | the patch passed |
| +1 :green_heart: | compile | 3m 55s | the patch passed |
| +1 :green_heart: | javac | 3m 55s | the patch passed |
| -0 :warning: | checkstyle | 1m 9s | hbase-server: The patch generated 24 new + 180 unchanged - 0 fixed = 204 total (was 180) |
| -0 :warning: | checkstyle | 0m 19s | hbase-it: The patch generated 62 new + 0 unchanged - 0 fixed = 62 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 26s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | spotbugs | 3m 7s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 53m 54s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4066 |
| JIRA Issue | HBASE-26707 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux a50b354deec4 4.15.0-161-generic #169-Ubuntu SMP Fri Oct 15 13:41:54 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / c33fa3ab78 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-it.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-it U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-4066/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.12.0 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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r799611300
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -390,11 +400,16 @@ public String prepareBulkLoad(final byte[] family, final String srcPath, boolean
LOG.debug("Moving " + p + " to " + stageP);
FileStatus origFileStatus = fs.getFileStatus(p);
origPermissions.put(srcPath, origFileStatus.getPermission());
+ origlSources.put(stageP.toString(), srcPath);
Review comment:
We could. My only concern is that it might be impractical over a certain data size especially if there is no config or param that would allow the user to use rename if they need to.
What do you think?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #4066: HBASE-26707: Reduce number of renames during bulkload
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #4066:
URL: https://github.com/apache/hbase/pull/4066#discussion_r800695112
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SecureBulkLoadManager.java
##########
@@ -341,27 +342,36 @@ private User getActiveUser() throws IOException {
return user;
}
- private static class SecureBulkLoadListener implements BulkLoadListener {
+ protected static class SecureBulkLoadListener implements BulkLoadListener {
// Target filesystem
private final FileSystem fs;
private final String stagingDir;
private final Configuration conf;
// Source filesystem
private FileSystem srcFs = null;
private Map<String, FsPermission> origPermissions = null;
+ private Map<String, String> origlSources = null;
Review comment:
> I left it like that because `origPermissions` uses String as well so a conversion would be done anyway. And while I could switch both of those to use Paths, log messages will need a string too.
Ok, let's just then follow var naming pattern, then.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org