HBASE-24625 AsyncFSWAL.getLogFileSizeIfBeingWritten does not return the expected synced file length. #1970
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 similar comment
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
retest build |
wchevreuil
reviewed
Jun 25, 2020
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
That is to say, AsyncFSWAL.getLogFileSizeIfBeingWritten could not reflect the file length which successfully synced to underlying HDFS, which is not as expected.
Wasn't that intentional, as a mean to proper track WAL files still open for write? For example, in case of replication, it should go as far as any entry got already appended, no? Ping @Apache9 who worked on this before to give more thoughts.
Apache9
requested changes
Jun 25, 2020
|
|
||
| protected FSDataOutputStream output; | ||
|
|
||
| private volatile long syncedLength = 0; |
Contributor
Author
There was a problem hiding this comment.
seems that using AtomicLong is unnecessary, because AtomicLong could not provide update if greater than semantics, so I used synchronized keyword here when updating the syncedLength for simplicity
Contributor
There was a problem hiding this comment.
Use AtomicUtils.updateMax. It is a util class in hbase-common.
Contributor
Author
There was a problem hiding this comment.
Used AtomicUtils to replace synchronized, thank you very much.
Member
There was a problem hiding this comment.
nit: why do we have AtomicUtils.updateMax? It seems getAndAccumulate is designed for this use case, i.e., syncedLength.getAndAccumulate(fsdos.getPos(), Math::max)
Contributor
This guy contacted me offline and I confirmed that this should be a problem. What I can recall is that, when doing some bug fixes and improving the performance in AsyncFSWAL, I changed the way we calculate the length of the writer. Maybe I forget the assumption in HBASE-14004 when doing these changes and lead to the problem. So @comnetwork , please add more comments to say why we need the getSyncedLength method in the WAL.Writer interface? So later people will not break it again. Thanks. |
Apache9
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 , I already added comments for /**
|
|
🎊 +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
requested changes
Jun 30, 2020
|
|
||
| protected FSDataOutputStream output; | ||
|
|
||
| private volatile long syncedLength = 0; |
Contributor
There was a problem hiding this comment.
Use AtomicUtils.updateMax. It is a util class in hbase-common.
Contributor
Author
|
@Apache9 , Used |
|
🎊 +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
Jul 2, 2020
Contributor
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
LGTM. But please fix the javadoc issues? After you fix the javadoc issues I will merge the PR.
Thanks.
| interface WriterBase extends Closeable { | ||
| long getLength(); | ||
| /** | ||
| * <pre> |
Contributor
There was a problem hiding this comment.
Do not need to use pre here? These are just normal text.
Contributor
Author
There was a problem hiding this comment.
ok, I would fix it.
Contributor
|
Hi, any updates here? @comnetwork |
Contributor
Author
|
Removed the pre tag in the comment and fix the javadoc bug. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
There are still checkstyle issues? |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Fixed the checkstyle errors. |
|
🎊 +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
Jul 4, 2020
…he expected synced file length. (#1970) Signed-off-by: Duo Zhang <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Jul 4, 2020
…he expected synced file length. (#1970) Signed-off-by: Duo Zhang <[email protected]>
ndimiduk
pushed a commit
to ndimiduk/hbase
that referenced
this pull request
Jul 6, 2020
…he expected synced file length. (apache#1970) Signed-off-by: Duo Zhang <[email protected]>
ndimiduk
reviewed
Jul 6, 2020
| future.complete(out.getPos()); | ||
| long pos = out.getPos(); | ||
| if(pos > this.syncedLength) { | ||
| this.syncedLength = pos; |
Member
There was a problem hiding this comment.
This read-followedby-update also needs to be atomic, yes?
Contributor
There was a problem hiding this comment.
This one is just for test so not a big problem but aligning with other producation implementations is better. Can have an addendum.
Contributor
There was a problem hiding this comment.
Oh, reviewed the code again, actuall, the flush0 method can only be executed in a single thread so no need to use AtomicUtils.updateMax. The AtomicLong is in the ProtobufLogWriter, not the output stream. But the 'if(pos > this.syncedLength) {' is a bit confusing to developers, I prefer we just remove this check...
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…he expected synced file length. (apache#1970) Signed-off-by: Duo Zhang <[email protected]>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…return the expected synced file length. (apache#1970)" This reverts commit f834919.
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…he expected synced file length. (apache#1970) Author: chenglei Reason: Bug Ref: CDPD-15964 Signed-off-by: Duo Zhang <[email protected]> Change-Id: Ie87d5e6f1eb47c48f413a31a0e5507b0090f5fe1 (cherry picked from commit e064d08)
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…he expected synced file length. (apache#1970) Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit bf587fa) Change-Id: Ie87d5e6f1eb47c48f413a31a0e5507b0090f5fe1
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…return the expected synced file length. (apache#1970)" This reverts commit bf587fa. (cherry picked from commit 160c229) Change-Id: Ifffde0910e6a219956697f417b76da31816a7852
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.
In the PR, I added a new method
getSyncedLengthtoWriterBaseinterface to return the length which successfully synced to underlying fileSystem.and
AbstractFSWAL.getLogFileSizeIfBeingWrittenis relied on the addedWriteBase.getSyncedLengthmethod: