HBASE-24625 AsyncFSWAL.getLogFileSizeIfBeingWritten does not return the expected synced file length(addendum) #2055
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
comnetwork
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
Apache9
requested changes
Jul 14, 2020
| private final Class<? extends Channel> channelClass; | ||
|
|
||
| private AsyncFSOutput output; | ||
| private volatile AsyncFSOutput output; |
Contributor
There was a problem hiding this comment.
Do we really need to make it volatile? IIRC, it will be initialized in the init call and we will always call it when constructing a new ProtobufLogWriter. Make it volatile may cause developer confusing...
Contributor
Author
There was a problem hiding this comment.
The output would be set to null in AsyncProtobufLogWriter.close and would be used in getSyncedLength method, and seems there is no explicit constraint on which thread could invoke AsyncProtobufLogWriter.close , so here use volatile
Contributor
There was a problem hiding this comment.
We will also use output in other methods where we do not test whether it is null, only test it in this method seems a bit confusing...
Contributor
Author
There was a problem hiding this comment.
Yes, you are right, I ignored other write methods also not test whether output is null. The problem here is it is meaningless to call AsyncProtobufLogWriter.getSyncedLength after AsyncProtobufLogWriter.close , which indeed occured when #1970 is applied to branch-2 previously caused by problematic state synchronization.
If we do not test whether output is null, we can just leave some comments here.
Contributor
There was a problem hiding this comment.
Oh, then this is another problem? I think it is allowed to call getSyncedLength after closing? We should not throw IllegalStateException...
Contributor
Author
There was a problem hiding this comment.
Yes ,the problem description is :
https://issues.apache.org/jira/browse/HBASE-24625?focusedCommentId=17152610&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17152610
In my opinion, AsyncProtobufLogWriter.getSyncedLength is used for WALFileLengthProvider.getLogFileSizeIfBeingWritten and is just meaningful when the WAL file is current writing, once AsyncProtobufLogWriter is closed(BTW, AsyncProtobufLogWriter.output is null), upperlayer code seems no need to call this method, and if upperlayer code still call this method after closed, it may indicates some synchronization error or other.
If we allow to call getSyncedLength after closing, we should save the syncedLength when AsyncProtobufLogWriter.close , or else getSyncedLength will throw NullPointException or just return 0, both are unexpected and may cause upperlayer code error.
Contributor
There was a problem hiding this comment.
In practise, as we will call getSyncedLength in another thread, it is possible that we call getSyncedLength on a closed writer. Maybe we could introduce another field to save the final length of the file, and in getSyncedLength, we check whether we have this field set, if so we just return the value of this field, otherwise we will go to get output. WDYT?
Contributor
Author
There was a problem hiding this comment.
@Apache9 , Yes, I agree with you, and would modify PR following your suggestion.
BTW, for now, AsyncProtobufLogWriter.getSyncedLength is only used when the WAL file is writing. AsyncProtobufLogWriter.getSyncedLength for closed wrter may be used for future use.
Contributor
Author
There was a problem hiding this comment.
@Apache9 , I modify PR to introduce finalSyncedLength to save the syncedLength when AsyncProtobufLogWriter.close.
But in getSyncedLength method, I check whether output is null instead of checking whether finalSyncedLength is set, because I think the statement "this.output = null;" in AsyncProtobufLogWriter.close is a good sync point, if output is null, then finalSyncedLength must set, so we can return finalSyncedLength, else we return output.getSyncedLength.
If we use finalSyncedLength as a sync point, it is hard to decide whether the output is null or not.
Contributor
Author
There was a problem hiding this comment.
@Apache9 , please have a review, thanks.
|
🎊 +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. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Aug 10, 2020
asfgit
pushed a commit
that referenced
this pull request
Aug 10, 2020
…he expected synced file length(addendum) (#2055) Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Aug 10, 2020
…he expected synced file length(addendum) (#2055) Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Aug 10, 2020
…he expected synced file length(addendum) (#2055) Signed-off-by: Duo Zhang <[email protected]>
|
🎊 +1 overall
This message was automatically generated. |
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…he expected synced file length(addendum) (apache#2055) Signed-off-by: Duo Zhang <[email protected]>
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…he expected synced file length(addendum) (apache#2055) Author: chenglei Reason: Bug Ref: CDPD-15964 Signed-off-by: Duo Zhang <[email protected]> Change-Id: I350de1bb30eb4378ba2da8adf6e20be25ebac5d3 (cherry picked from commit 957ddda)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…he expected synced file length(addendum) (apache#2055) Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 52933e2) Change-Id: I350de1bb30eb4378ba2da8adf6e20be25ebac5d3
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Open this addendum PR to incorporate @ndimiduk 's suggestions in #2034 and other minor changes.
I did not use the
AtomicLong.getAndAccumulateto replaceAtomicUtils.updateMaxbecausehttps://issues.apache.org/jira/browse/HBASE-24625?focusedCommentId=17155993&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17155993
@Apache9 @ndimiduk