-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-24740 Enable journal logging for HBase snapshot operation #2104
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
Conversation
|
🎊 +1 overall
This message was automatically generated. |
|
@bharathv Please review and merge |
bharathv
left a comment
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.
A few nits, but lgtm, can commit once fixed. @sguggilam Can you please confirm that you did a quick sanity check on the journal logging? (given there are no tests).
| false); | ||
| } else { | ||
| LOG.debug("Convert to Single Snapshot Manifest"); | ||
| LOG.debug("Convert to Single Snapshot Manifest for " + this.desc.getName()); |
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.
nit: parameterized logging
| if (master.getTableStateManager().isTableState(snapshotTable, | ||
| TableState.State.ENABLED)) { | ||
| LOG.debug("Table enabled, starting distributed snapshot."); | ||
| LOG.debug("Table enabled, starting distributed snapshots for {}", |
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.
nit: This still requires the isDebugEnabled() guard because ClientSnapshotDescriptionUtils.toString(..) is not a trivial call.
| else if (master.getTableStateManager().isTableState(snapshotTable, | ||
| TableState.State.DISABLED)) { | ||
| LOG.debug("Table is disabled, running snapshot entirely on master."); | ||
| LOG.debug("Table is disabled, running snapshot entirely on master for {}", |
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.
same as above.
|
Yes @bharathv , tested out the journal log entry and below is the snippet Table snapshot journal : |
bharathv
left a comment
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.
Perfect, thanks. Will merge it once the precommit is green.
|
💔 -1 overall
This message was automatically generated. |
Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 430602a)
Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 430602a)
Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 430602a)
|
💔 -1 overall
This message was automatically generated. |
…he#2104) Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 430602a)
…he#2104) Signed-off-by: Bharath Vissapragada <[email protected]> (cherry picked from commit 430602a) (cherry picked from commit 4e5ec22) Change-Id: Ibe837047e9f93c608ec13972a9f6b1ac3622c92f
No description provided.