HBASE-24822 Add a command to support archive the earliest log file ma… #2202
+347
−2
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
The remained checkstyle issue is unrelated to this PR. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
tedyu
reviewed
Aug 7, 2020
| * @param serverName The servername of the regionserver. | ||
| * @throws IOException if a remote or network exception occurs | ||
| */ | ||
| void archiveWAL(ServerName serverName) throws IOException; |
There was a problem hiding this comment.
Should method name reflect that the earliest log is archived ?
There was a problem hiding this comment.
Good point, will fix later.
Thanks.
| .build(); | ||
|
|
||
| /** | ||
| * Create a new ArchiveWALRequest |
There was a problem hiding this comment.
Will fix later.
Thanks.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
rebase |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The failure seems unrelated to this PR. |
|
PTAL if you have time. Thanks. @tedyu |
tedyu
reviewed
Aug 10, 2020
| @@ -219,6 +219,12 @@ message RollWALWriterResponse { | |||
| repeated bytes region_to_flush = 1; | |||
| } | |||
|
|
|||
| message ArchiveWALRequest { | |||
There was a problem hiding this comment.
Should this be renamed to go with the updated method name ?
There was a problem hiding this comment.
Will fix later. Thanks.
There was a problem hiding this comment.
Thought again, it should be a common name if we want to improve it in future, such as archive a specify log file, archive all log files, etc.
WDYT? Thanks.
There was a problem hiding this comment.
If it make sense, i will remove the "earliest" from method name also.
There was a problem hiding this comment.
I think ArchiveWALResponse should contain the status of archival (including error).
There was a problem hiding this comment.
I think ArchiveWALResponse should contain the status of archival (including error).
Make sense.
There was a problem hiding this comment.
Checked the existing api in Admin.java, such as flushRegion, compactRegion, seems currently we dont use status, if there is something wrong at server side, we notice client by throw ServiceException.
Though the FlushRegionResponse has a flushed flag, but does not be used at client.
Maybe we should keep consistent?
Thanks for comment. @tedyu
| } | ||
| // move the log file to archive dir | ||
| this.totalLogSize.addAndGet(-firstWALEntry.getValue().logSize); | ||
| moveLogFileToArchiveDir(firstWALEntry.getKey()); |
There was a problem hiding this comment.
What if IOException is thrown from this call ?
Would totalLogSize be correct ?
There was a problem hiding this comment.
Good point, the move action should be called first.
BTW, this part copied from existing code, maybe need a follow-on jira to fix it too.
Thanks.
| if (r == null) { | ||
| LOG.warn("Failed to flush of {} when archive manually, because it is not online on us", | ||
| encodedRegionName); | ||
| return; |
There was a problem hiding this comment.
It seems the return value should not be void - otherwise the caller wouldn't know this scenario (region not found).
There was a problem hiding this comment.
What do you think of just continue, since the region is not online(maybe closed or moved), anyway we will not lost data.
There was a problem hiding this comment.
When region close, the onRegionClose of SequenceIdAccounting should be called, so its regions will consist with onlineRegions in HRegionServer, if not, there must be something wrong, then the onlineRegions shall prevail.
There was a problem hiding this comment.
Or throw an Exception to notice client?
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Ping. @tedyu |
|
I am a bit busy recently. Please find another committer to review. Thanks |
Ok, thanks all the same for the comments. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
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.
…nually