HBASE-25891 remove dependence storing wal filenames for backup #3359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
rda3mon
force-pushed
the
HBASE-25891-remove-dependence-storing-wal-filenames-for-backup
branch
from
f0f898f
to
fca1502
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
rda3mon
force-pushed
the
HBASE-25891-remove-dependence-storing-wal-filenames-for-backup
branch
from
fca1502
to
c4ee13a
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 Can you review this? |
|
Will try to take a look tomorrow. It is a bit late in China, sorry... |
No problem. Have a look when you find some time. Thanks |
|
@Apache9 Gentle reminder. |
saintstack
approved these changes
Jul 27, 2021
| * safely purged. | ||
| */ | ||
| public void recordWALFiles(List<String> files) throws IOException { | ||
| systemTable.addWALFiles(files, backupInfo.getBackupId(), backupInfo.getBackupRootDir()); |
There was a problem hiding this comment.
Can we get rid of the 'system' table altogether?
There was a problem hiding this comment.
Let me review and come back on the feasibility aspect.
There was a problem hiding this comment.
Did you figure anything @rda3mon ?
There was a problem hiding this comment.
@saintstack Sorry, I missed to reply on this one.
There is more cleanup possible, but not everything can be removed. Do you think It can be merged to hbase:meta? Or is there a recommendation on what should be in hbase:meta and what shouldn't?
There was a problem hiding this comment.
Created a ticket on this --> https://issues.apache.org/jira/browse/HBASE-26279
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupManifest.java
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Show resolved
Hide resolved
Apache9
reviewed
Jul 28, 2021
| this.tableSetTimestampMap = newTableSetTimestampMap; | ||
| public void setIncrTimestampMap(Map<TableName, | ||
| Map<String, Long>> prevTableSetTimestampMap) { | ||
| this.incrTimestampMap = prevTableSetTimestampMap; |
There was a problem hiding this comment.
Is this a bug in the old code? We do not change the name of the method but change the implementation?
There was a problem hiding this comment.
It looks like. What do you suggest to do here? Since backup is not part of any release, I took the liberty to correct this. But open for suggestions.
There was a problem hiding this comment.
incrTimestampMap is used by incremental backup to check when was previous backup logroll happened so that it can take backup since then. and stored as part of backup manifest.
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/BackupSystemTable.java
Show resolved
Hide resolved
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java
Show resolved
Hide resolved
|
Do you want to push a new PR here @rda3mon ? |
|
@saintstack I dont have anything for this one. Unless there are some suggestions. Can we merge if no comments? @Apache9 ? |
|
@Apache9 @saintstack If there are no suggestions, can we merge the change? I have followup patch ready to raise PR --> https://issues.apache.org/jira/browse/HBASE-25784 |
|
I'm not very familiar with the code so for me I would like to trust your judgement @rda3mon , I have no more questions here. There is still a pending question from @saintstack about whether it is OK to remove the 'system' table, let's wait for his answer first. Thanks. |
Apache9
approved these changes
Aug 29, 2021
|
Thanks @Apache9. Will wait for @saintstack to comment. |
|
@saintstack Gentle Reminder. |
|
@Apache9 Since @saintstack comment was more on direction than on this particular PR. I have create ticket to track it --> https://issues.apache.org/jira/browse/HBASE-26279. Should we merge these changes as @saintstack has already approved the PR? I have follow up changes on parallel backup for which I can open a PR. |
|
Will merge tomorrow if still no other responses and objections. Thank you for your patient @rda3mon . |
rda3mon
deleted the
HBASE-25891-remove-dependence-storing-wal-filenames-for-backup
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.