Uh oh!
There was an error while loading. Please reload this page.
HBASE-26874 VerifyReplication recompare async - #5051
Merged
Merged
Conversation
bbeaudreault
self-requested a review
February 21, 2023 19:26
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Uh oh!
There was an error while loading. Please reload this page.
| if (verbose) { | ||
| LOG.info("Good row key (with recompare): " + delimiter | ||
| + Bytes.toStringBinary(row.getRow()) + delimiter); | ||
| LOG.info("Got an exception during recompare for rowkey=" + Bytes.toStringBinary(row), |
Contributor
There was a problem hiding this comment.
we have slf4j here, so can use {} placeholders in all of these new LOG calls
Uh oh!
There was an error while loading. Please reload this page.
| return new CallerRunsPolicy() { | ||
| @Override | ||
| public void rejectedExecution(Runnable runnable, ThreadPoolExecutor e) { | ||
| LOG.info("Re-comparison execution rejected. Running in main thread."); |
Contributor
There was a problem hiding this comment.
might want to make this debug. In general best to avoid info logs in M/R jobs because they can unexpected slow down jobs if they start spamming
| + Bytes.toStringBinary(row.getRow()) + delimiter); | ||
| if (failCounter != null) { | ||
| context.getCounter(failCounter).increment(1); | ||
| LOG.debug(failCounter + " for rowkey=" + Bytes.toStringBinary(row)); |
Contributor
There was a problem hiding this comment.
wrap this in LOG.isDebugEnabled, since converting bytes to StringBinary takes cpu
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| // we use a static retry/backoff schedule for this part because | ||
| // we have an opportunity rerun the fetching again when we retry the | ||
| // higher level recompare as well. | ||
| private static final int FETCH_LATEST_ROWS_TRIES = 3; |
Contributor
There was a problem hiding this comment.
Taking another look at this. In terms of upstreaming here, i feel like we might want to simplify this. I don't love having something unconfigurable like this, since it's just begging for someone to need to push another update later to make it configurable. I also think it's a little harder for some new user to reason about a design which makes use of two retries like we have(a high level and lower level retry).
Rather than have 2 retries, can we make do with 1? That would require some modifications to the code below. It would also probably mean configuring our internal wrapper job to have higher number of retries than we have today.
So basically I think we'd want to have the 1 loop in RecompareRunnable and maybe change fetchLatestRows to return a boolean which causes us to have retry again in the main loop.
Does that make sense?
ContributorAuthor
There was a problem hiding this comment.
Yeah that makes sense, I can implement these changes 👍
| LOG.error("{}, rowkey={}{}{}", counter, delimiter, Bytes.toStringBinary(rowKey), delimiter); | ||
| } | ||
| private class RecompareRunnable implements Runnable { |
Contributor
There was a problem hiding this comment.
The other thing I'm thinking is whether we can split this out to an upper level class, i.e. its own class named VerifyReplicationRecompareRunnable. I just think this job is complicated and long, so adding an entire inner class here is a non-trivial addition to the complexity. It'd also make it possible to write unit tests just against the recompare process, without having to worry about somehow running the full MR job and trying to cause inconsistencies.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
hgromer
commented
Mar 21, 2023
ContributorAuthor
Looking into those issues ^ |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
hgromer
commented
Jul 28, 2023
ContributorAuthor
^ looking into these failures |
Apache-HBase
commented
Jul 28, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2023
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
requested changes
Jul 29, 2023
bbeaudreault
left a comment
Contributor
There was a problem hiding this comment.
Looks great! I had a few super minor things, and then 1 change to how we setup the default state. We should be good to go after these
Uh oh!
There was an error while loading. Please reload this page.
| throws IOException { | ||
| if (replicatedScanner == null) { | ||
| Configuration conf = context.getConfiguration(); | ||
| reCompareTries = conf.getInt(NAME + ".recompareTries", 1); |
Contributor
There was a problem hiding this comment.
actually im realizing we might want to make this dynamic --
- from the perspective of backwards compatibility, by default there were no retries. so a default value of 0 makes sense.
- previously, the way to specify retries was to set sleepMsBeforeReCompare and it would always just do 1 retry
so i think we should default this to 0, and then below add:
if (sleepMsBeforeReCompare > 0) {
reCompareTries = Math.max(reCompareTries, 1);
}meaning, we've now aligned the these values to make sense together. If someone explicitly configures more retries, they'll get them. And otherwise they will get 1 retry if they specified sleepMsBeforeReCompare or 0 if not, like it used to work.
Then, way below where you currently decide whether to create a runnable or not, you should base it on reCompareTries == 0 rather than sleepMsBeforeReCompare == 0.
I think this makes a little more intuitive sense, since what it's basing the logic on whats actually happening -- whether there are retries are not, rather than what the backoff is for those retries. The other benefit is if someone for some reason wanted to try 3 times with zero backoff, currently they can't do that but in the new setup they would.
ContributorAuthor
There was a problem hiding this comment.
good point, I think those changes make sense!
| } | ||
| } | ||
| } catch (InterruptedException e) { | ||
| LOG.error("fail to await executor termination in cleanup", e); |
Contributor
There was a problem hiding this comment.
i think we need to at least increment BADROWS/FAILED_RECOMPARE here, or otherwise throw an exception. I think throwing an exception probably makes sense, since we have no idea what the state of pending recompares was
ContributorAuthor
There was a problem hiding this comment.
opted for throwing b/c as you mentioned we don't really have any idea what's going on within the executor at the time
| if (cmd.startsWith(sleepToReCompareKey)) { | ||
| sleepMsBeforeReCompare = Integer.parseInt(cmd.substring(sleepToReCompareKey.length())); | ||
| sleepMsBeforeReCompare = | ||
| Integer.parseInt(cmd.substring(deprecatedSleepToReCompareKey.length())); |
Contributor
There was a problem hiding this comment.
i think you meant to substring sleepToReCompareKey here. I realize it's the same length, but just to avoid errors/confusion in the future.
ContributorAuthor
There was a problem hiding this comment.
ah you're totally right, this was my bad
Uh oh!
There was an error while loading. Please reload this page.
hgromerforce-pushed
the
HBASE-26874
branch
2 times, most recently
from
July 31, 2023 17:29
10f1677 to
b43626dCompareApache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
| private void logFailRowAndIncreaseCounter(Context context, Counters counter, Result row, | ||
| Result replicatedRow) { | ||
| byte[] rowKey = getRow(row, replicatedRow); | ||
| if (sleepMsBeforeReCompare == 0) { |
Contributor
There was a problem hiding this comment.
change this to recompareTries == 0
Apache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
🎊 +1 overall
This message was automatically generated. |
bbeaudreault pushed a commit
that referenced
this pull request
Aug 1, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault pushed a commit
that referenced
this pull request
Aug 1, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault pushed a commit
to HubSpot/hbase
that referenced
this pull request
Aug 1, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
ndimiduk
commented
Aug 7, 2023
Member
Hey team. This commit hit the repo without the Jira ID in the commit title. Please revert and recommit with a corrected title. Thanks! |
bbeaudreault
commented
Aug 7, 2023
Contributor
Ugh!!! I don't know how that happened, I usually edit the commit message before merging to ensure. Will fix |
bbeaudreault added a commit
that referenced
this pull request
Aug 7, 2023
This reverts commit f6c5dbe.
bbeaudreault pushed a commit
that referenced
this pull request
Aug 7, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
bbeaudreault added a commit
that referenced
this pull request
Aug 7, 2023
This reverts commit 2b3d322.
bbeaudreault pushed a commit
that referenced
this pull request
Aug 7, 2023
Signed-off-by: Bryan Beaudreault <bbeaudreault@apache.org>
Apache9 pushed a commit
that referenced
this pull request
Nov 21, 2024
Signed-off-by: Bryan Beaudreault <bbeaudreault@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.
No description provided.