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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
