Uh oh!
There was an error while loading. Please reload this page.
HBASE-28184 Tailing the WAL is very slow if there are multiple peers - #5503
Merged
Conversation
Apache-HBase
commented
Nov 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 4, 2023
Apache9
left a comment
Contributor
There was a problem hiding this comment.
The code change LGTM, but let's rephrase the comments there
OptionalLong fileLength;
if (logQueue.getQueueSize(walGroupId) > 1) {
// if there are more than one files in queue, although it is possible that we are
// still trying to write the trailer of the file and it is not closed yet, we can
// make sure that we will not write any WAL entries to it any more, so it is safe
// to just let the upper layer try to read the whole file without limit
fileLength = OptionalLong.empty();
} else {
// if there is only one file in queue, check whether it is still being written to
// we must call this before actually reading from the reader, as this method will acquire the
// rollWriteLock. This is very important, as we will enqueue the new WAL file in postLogRoll,
// and before this happens, we could have already finished closing the previous WAL file. If
// we do not acquire the rollWriteLock and return whether the current file is being written
// to, we may finish reading the previous WAL file and start to read the next one, before it
// is enqueued into the logQueue, thus lead to an empty logQueue and make the shipper think
// the queue is already ended and quit. See HBASE-28114 and related issues for more details.
// in the future, if we want to optimize the logic here, for example, do not call this method
// every time, or do not acquire rollWriteLock in the implementation of this method, we need
// to carefully review the optimized implementation
fileLength = walFileLengthProvider.getLogFileSizeIfBeingWritten(currentPath);
}
shahrs87
commented
Nov 4, 2023
ContributorAuthor
Thank you @Apache9 for your feedback and your requested comments are very detailed. Made the changes. Can you please review again? Thank you ! |
Apache-HBase
commented
Nov 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 4, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Nov 5, 2023
Apache-HBase
commented
Nov 5, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 6, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 6, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Nov 6, 2023
Contributor
The code is bit different on branch-2.5 and branch-2.4, please open a separated PR for against branch-2.5? Thanks. |
ContributorAuthor
virajjasani
approved these changes
Nov 6, 2023
Comment on lines
470
to
472
| // in the future, if we want to optimize the logic here, for example, do not call this method | ||
| // every time, or do not acquire rollWriteLock in the implementation of this method, we need to | ||
| // carefully review the optimized implementation |
Contributor
There was a problem hiding this comment.
nit: now that we have comments for both if and else, maybe good to remove from here?
sunhelly
approved these changes
Nov 6, 2023
Apache-HBase
commented
Nov 6, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 7, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 7, 2023
🎊 +1 overall
This message was automatically generated. |
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
Sign up for freeto 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.