HBASE-24182 log when the region is set to closing status #1509
Conversation
This file contains hidden or 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. |
virajjasani
requested changes
Apr 14, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
We are already logging close region journal as per HBASE-22828. No need to have additional info log.
Contributor
|
@virajjasani No strong opinion from me but the one new INFO level log inline with other logging seems fine. The close journal is printed after the fact and so is a bit confusing if you are sorting log lines chronologically. Sure, overall the logging is redundant. |
Contributor
Sure, for the sake of chronological sorting, it is good to log this. Let's get it. |
virajjasani
reviewed
Apr 17, 2020
| } | ||
| } | ||
| this.closing.set(true); | ||
| LOG.info("Closing {}", this); |
Contributor
There was a problem hiding this comment.
@Joseph295 Can you change this to "Closing region {}"? this is anyways going to print region name as string only.
Contributor
Author
There was a problem hiding this comment.
@Joseph295 Can you change this to "Closing region {}"?
thisis anyways going to print region name as string only.
I think the discussion above really makes sense, so do you guys think we add this log any more? maybe for debug level is enough? We only need it if we concern about the closing cost time for debug? Anyway, let me fix the format for that, any suggestion is welcome. Thanks
Contributor
There was a problem hiding this comment.
Journal log is at Debug level anyways right? Let this be Info level, should be good. Imp point here is of timing of these logs and hence, if we debug from logs based on region name, we can see sorting of chronological events one after another and that way having this Info log is beneficial (although redundant).
Let's wait for @apurtell 's final call on this.
virajjasani
approved these changes
Apr 17, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Updating my vote to +1
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Apr 17, 2020
Signed-off-by: stack <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Apr 17, 2020
Signed-off-by: stack <[email protected]>
ddupg
pushed a commit
to ddupg/hbase
that referenced
this pull request
Apr 20, 2020
Signed-off-by: stack <[email protected]>
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
Signed-off-by: stack <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.