-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28539 Merge of incremental backups fails if backups are on a separate FileSystem #5867
HBASE-28539 Merge of incremental backups fails if backups are on a separate FileSystem #5867
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Please run mvn spotless:apply
to fix styling problems.
cc47335
to
0b6738e
Compare
@anmolnar applied as requested |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had one minor suggestion, but this also looks good to me
File tempDir = new File(FileUtils.getTempDirectory(), UUID.randomUUID().toString()); | ||
tempDir.deleteOnExit(); | ||
BACKUP_ROOT_DIR = tempDir.toURI().toString(); | ||
System.out.println(BACKUP_ROOT_DIR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not totally unheard of to print logs like this, especially in our test files, but maybe we should just use the logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmdmattingly I've removed the offending debug statement
0b6738e
to
711535b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core change seems good to me. I have some test reliability requests.
@@ -124,4 +128,43 @@ public void TestIncBackupMergeRestore() throws Exception { | |||
admin.close(); | |||
conn.close(); | |||
} | |||
|
|||
@Test | |||
public void TestIncBackupMergeRestoreSeparateFs() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, strange. We don't usually use C# casing in Java code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the method above, but now changed it to typical Java standard.
|
||
// prepare BACKUP_ROOT_DIR on a different filesystem from HBase | ||
File tempDir = new File(FileUtils.getTempDirectory(), UUID.randomUUID().toString()); | ||
tempDir.deleteOnExit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Over the years, we've found that deleteOnExit
is unreliable for cleaning up unit tests. Better to allocate a path under the maven target
directory instead. I believe that we have utility methods to accomplish this as part of HBaseTestingUtility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tempDir.deleteOnExit(); | ||
BACKUP_ROOT_DIR = tempDir.toURI().toString(); | ||
|
||
Connection conn = ConnectionFactory.createConnection(conf1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use try-with-resources to close the connection. Actually I'm surprised that we don't have a static analysis tool warning about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@ndimiduk |
711535b
to
e07a599
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Heya @bcolyn-ngdata can you please run |
…stem is not the same as the one underpinning HBase itself.
e07a599
to
1d18900
Compare
Hi @ndimiduk I've run the spotless:apply (sorry, should have done that already) and amended the PR |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…parate FileSystem (apache#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
…parate FileSystem (apache#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
…parate FileSystem (apache#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
…parate FileSystem (#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
…parate FileSystem (#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
…parate FileSystem (#5867) When the backups are stored on a location that is not the DistributedFilesystem underpinning HBase itself merging of incremental backups fails. Detected with backups stored on S3A, but can be reproduced with any other (like LocalFilesystem). Reviewed-by: Ray Mattingly <rmdmattingly@gmail.com> Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Andor Molnár <andor@apache.org>
HBASE-28539 Fix merging of incremental backups when the backup filesystem is not the same as the one underpinning HBase itself.