ctubbsii commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r468796137
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/Master.java
##########
@@ -1422,7 +1423,8 @@ private void getMasterLock(final String zMasterLoc)
throws KeeperException, Inte
while (true) {
MasterLockWatcher masterLockWatcher = new MasterLockWatcher();
- masterLock = new ZooLock(context.getZooReaderWriter(), zMasterLoc);
+ masterLock =
+ new ZooLock(context.getZooReaderWriter(), zMasterLoc,
ZooReader.DISABLED_RETRY_FACTORY);
Review comment:
In these kinds of uses, the code reads as though ZooLock itself won't
retry... this is a bit unintuitive, because of the unfortunate name.
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -69,13 +70,14 @@
private boolean watchingParent = false;
private String asyncLock;
- public ZooLock(ZooReaderWriter zoo, String path) {
+ public ZooLock(ZooReaderWriter zoo, String path, RetryFactory retryFactory) {
Review comment:
A new parameter is added, but not used.
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java
##########
@@ -38,24 +38,36 @@
public class ZooReader {
private static final Logger log = LoggerFactory.getLogger(ZooReader.class);
- protected static final RetryFactory RETRY_FACTORY =
Retry.builder().maxRetries(10)
+ public static final RetryFactory DEFAULT_RETRY_FACTORY =
Retry.builder().maxRetries(10)
+ .retryAfter(250, MILLISECONDS).incrementBy(250, MILLISECONDS).maxWait(5,
TimeUnit.SECONDS)
+ .backOffFactor(1.5).logInterval(3, TimeUnit.MINUTES).createFactory();
+
+ public static final RetryFactory DISABLED_RETRY_FACTORY =
Retry.builder().maxRetries(0)
.retryAfter(250, MILLISECONDS).incrementBy(250, MILLISECONDS).maxWait(5,
TimeUnit.SECONDS)
.backOffFactor(1.5).logInterval(3, TimeUnit.MINUTES).createFactory();
Review comment:
Can probably remove all this extra retry configuration since the max
retries is zero here. I don't think these are all required parameters are they?
##########
File path:
core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java
##########
@@ -49,13 +49,14 @@
public ZooReaderWriter(AccumuloConfiguration conf) {
this(conf.get(Property.INSTANCE_ZK_HOST),
(int) conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.INSTANCE_SECRET));
+ conf.get(Property.INSTANCE_SECRET), ZooReader.DISABLED_RETRY_FACTORY);
Review comment:
The behavioral differences between the constructors is unintuitive. Can
we instead use a well-named static factory method, instead of the constructor
to construct a ZRW for the specific intended purpose, so the constructor isn't
unintentionally used by other code, and subject to the non-retrying behavior
accidentally?
```java
// e.g.
ZooReaderWriter zrw1 = ZooReaderWriter.retrying(conf);
ZooReaderWriter zrw2 = ZooReaderWriter.nonRetrying(conf);
// or
ZooReaderWriter zrw = ZooReaderWriter.forXuseOnly(conf);
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
