[GitHub] [hbase] bbeaudreault commented on a diff in pull request #5051: HBASE-26874 VerifyReplication recompare async

["bbeaudreault (via GitHub)" <gi...@apache.org>]

bbeaudreault commented on code in PR #5051:
URL: https://github.com/apache/hbase/pull/5051#discussion_r1129759295


##########
hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/replication/VerifyReplication.java:
##########
@@ -112,6 +123,12 @@ public class VerifyReplication extends Configured implements Tool {
   // Peer Table Name
   String peerTableName = null;
 
+  // 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;

Review 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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org