-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-28184 Tailing the WAL is very slow if there are multiple peers #5503
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
Thank you @Apache9 for your feedback and your requested comments are very detailed. Made the changes. Can you please review again? Thank you ! |
🎊 +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. |
The code is bit different on branch-2.5 and branch-2.4, please open a separated PR for against branch-2.5? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: now that we have comments for both if and else, maybe good to remove from here?
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.