Uh oh!
There was an error while loading. Please reload this page.
[HBASE-25539] Add age of oldest wal metric - #2945
Merged
Merged
Conversation
shahrs87
marked this pull request as draft
February 9, 2021 20:22
Apache-HBase
commented
Feb 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 9, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 10, 2021
💔 -1 overall
This message was automatically generated. |
| @@ -0,0 +1,93 @@ | |||
| package org.apache.hadoop.hbase.replication.regionserver; | |||
ContributorAuthor
There was a problem hiding this comment.
@saintstack This is still in draft mode. Wanted to run tests first before I submit for review. Thank you !
| @InterfaceAudience.Private | ||
| @InterfaceStability.Evolving | ||
| public class ReplicationLogQueue { |
Contributor
There was a problem hiding this comment.
Class comment on what this thing does
ContributorAuthor
There was a problem hiding this comment.
@saintstack This is still in draft mode. Wanted to run tests first before I submit for review. Thank you !
| // Queues of logs to process, entry in format of walGroupId->queue, | ||
| // each presents a queue for one wal group | ||
| private Map<String, PriorityBlockingQueue<Path>> queues = new HashMap<>(); | ||
| //private Map<String, PriorityBlockingQueue<Path>> queues = new HashMap<>(); |
Contributor
There was a problem hiding this comment.
Remove rather than comment out (This is a draft so maybe you are just trying stuff -- if so, ignore this comment)
Apache-HBase
commented
Feb 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 10, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
marked this pull request as ready for review
February 10, 2021 20:00
shahrs87
commented
Feb 10, 2021
ContributorAuthor
@saintstack This is ready for review now. Please review. |
Apache-HBase
commented
Feb 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 10, 2021
💔 -1 overall
This message was automatically generated. |
shahrs87
commented
Feb 11, 2021
ContributorAuthor
@bharathv@gjacoby126 could you guys also help reviewing this change. Thank you ! |
Apache-HBase
commented
Feb 11, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 11, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 11, 2021
🎊 +1 overall
This message was automatically generated. |
bharathv
left a comment
Contributor
There was a problem hiding this comment.
Nice refactoring, have a few minor comments.
Uh oh!
There was an error while loading. Please reload this page.
| import org.slf4j.LoggerFactory; | ||
| /* | ||
| Class that does enqueueing/dequeueing of wal at one place so that we can update the metrics |
| // WARN threshold for the number of queued logs, defaults to 2 | ||
| private int logQueueWarnThreshold; | ||
| private ReplicationSource source; | ||
| private static final Logger LOG = LoggerFactory.getLogger(ReplicationSource.class); |
Contributor
There was a problem hiding this comment.
nit: static finals to the top of the class.
| // the shipper may quit immediately | ||
| queue.put(wal); | ||
| queues.put(walPrefix, queue); | ||
| boolean queueAlreadyExisted = logQueue.enqueueLog(wal, walPrefix); |
| */ | ||
| public boolean enqueueLog(Path wal, String walGroupId) { | ||
| boolean exists = false; | ||
| PriorityBlockingQueue<Path> queue = queues.get(walGroupId); |
Contributor
There was a problem hiding this comment.
I'm a bit concerned that this is not thread-safe. It wasn't before the patch too but this seems prone to weird concurrent modification issues. Fix while we are here?
ContributorAuthor
There was a problem hiding this comment.
I changed queues implementation from HashMap to ConcurrentHashMap. I thought your concern was regarding queues data structure. But reading the comment again, think you were concerned about PriorityBlockingQueue within map ?
| private static final Logger LOG = LoggerFactory.getLogger(ReplicationSource.class); | ||
| public ReplicationLogQueue(Configuration conf, MetricsSource metrics, ReplicationSource source) { |
Contributor
There was a problem hiding this comment.
nit: thinking out loud, should we rename it to ReplicationSourceLogQueue to better convey that this is per source across all walGroups?
| /* | ||
| Get the oldest wal timestamp from all the queues. | ||
| */ |
| private void setOldestWalAge() { | ||
| long now = EnvironmentEdgeManager.currentTime(); | ||
| long timestamp = getOldestWalTimestamp(); | ||
| // TODO: Should we handle the case where getOldestWalTimestamp returns Long.MAX_VALUE ? |
Contributor
There was a problem hiding this comment.
think we should? Otherwise on empty queue we get false alarms?
ContributorAuthor
There was a problem hiding this comment.
AFAICT there would be atleast one source with active wal at the head of the queue always. In any case add code to handle Long.MAX_VALUE scenario.
| */ | ||
| private long getOldestWalTimestamp() { | ||
| long oldestWalTimestamp = Long.MAX_VALUE; | ||
| for (Map.Entry<String, PriorityBlockingQueue<Path>> entry : queues.entrySet()) { |
Contributor
There was a problem hiding this comment.
this is an O(n) loop but should be ok because the no. of walGroups is typically in a few 100s max?
ContributorAuthor
There was a problem hiding this comment.
Yes. That is the reason I was ok with O(n) loop.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 12, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Feb 12, 2021
ContributorAuthor
@bharathv Tried to address all of your comments. Could you please review again ? Thank you ! |
Apache-HBase
commented
Feb 12, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 13, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 13, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Feb 15, 2021
ContributorAuthor
@bharathv could you please review once again ? Thank you ! |
| Testing enqueue and dequeuing of wal and check age of oldest wal. | ||
| */ | ||
| @Test | ||
| public void testEnqueueDequeue() { |
Contributor
There was a problem hiding this comment.
Looks like this test includes everything tested by testAgeOfOldestWal() above, get rid of the above one or club them? (unless I'm missing something)..
ContributorAuthor
There was a problem hiding this comment.
TestReplicationSource#testAgeOfOldestWal brings up an actual cluster and starts a new ReplicationSource so I thought to keep it but enqueuing and dequeuing happens at separate places so it was hard to test dequeue logic in TestReplicationSource.TestReplicationSourceLogQueue is an unit test where enqueuing and dequeuing is easy to test but it doesn't bring up any cluster. So I thought to keep both. I don't have any strong opinions if you want me to delete one test. I would prefer to delete TestReplicationSource#testAgeOfOldestWal since we are testing more stuff in TestReplicationSourceLogQueue.
| */ | ||
| public boolean enqueueLog(Path wal, String walGroupId) { | ||
| boolean exists = false; | ||
| PriorityBlockingQueue<Path> queue = queues.get(walGroupId); |
bharathv
approved these changes
Feb 17, 2021
bharathv
left a comment
Contributor
There was a problem hiding this comment.
@saintstack Do you have any more comments on this?
bharathv
commented
Feb 18, 2021
Contributor
@shahrs87 There is a minor conflict on this one, mind rebasing and then I can commit. Thanks. |
shahrs87
commented
Feb 18, 2021
ContributorAuthor
Apache-HBase
commented
Feb 18, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 18, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 18, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Feb 18, 2021
ContributorAuthor
Ran failing test
|
ddupg pushed a commit
to ddupg/hbase
that referenced
this pull request
Feb 19, 2021
Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
Feb 24, 2021
Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
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
4 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.
https://issues.apache.org/jira/browse/HBASE-25539