we have slf4j here, so can use {} placeholders in all of these new LOG calls

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

wrap this in LOG.isDebugEnabled, since converting bytes to StringBinary takes cpu

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?

Yeah that makes sense, I can implement these changes 👍

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.

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.

good point, I think those changes make sense!

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

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

i think you meant to substring sleepToReCompareKey here. I realize it's the same length, but just to avoid errors/confusion in the future.

ah you're totally right, this was my bad

change this to recompareTries == 0