-
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-25891 remove dependence storing wal filenames for backup #3359
HBASE-25891 remove dependence storing wal filenames for backup #3359
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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. |
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. |
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.
Skimmed. LGTM. I like the undoing of dependence on backup system table. Would like to see the backup table go away completly if possible. Would like backup to be an app on top of hbase rather than integral too.. That said, looks like nice cleanup.
* safely purged. | ||
*/ | ||
public void recordWALFiles(List<String> files) throws IOException { | ||
systemTable.addWALFiles(files, backupInfo.getBackupId(), backupInfo.getBackupRootDir()); |
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.
Can we get rid of the 'system' table altogether?
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.
Let me review and come back on the feasibility aspect.
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.
Did you figure anything @rda3mon ?
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Not very familiar with the code so asked some dumb question...
this.tableSetTimestampMap = newTableSetTimestampMap; | ||
public void setIncrTimestampMap(Map<TableName, | ||
Map<String, Long>> prevTableSetTimestampMap) { | ||
this.incrTimestampMap = prevTableSetTimestampMap; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 . |
No description provided.