You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/08/11 17:33:26 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #1677: fixes #1086 - Disable retries from the server components
dlmarion opened a new pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469468185
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReaderWriter.java
##########
@@ -46,16 +46,32 @@
private static final Logger log = LoggerFactory.getLogger(ZooReaderWriter.class);
- public ZooReaderWriter(AccumuloConfiguration conf) {
- this(conf.get(Property.INSTANCE_ZK_HOST),
+ public static ZooReaderWriter retriesEnabled(AccumuloConfiguration conf) {
+ return new ZooReaderWriter(conf.get(Property.INSTANCE_ZK_HOST),
(int) conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
- conf.get(Property.INSTANCE_SECRET));
+ conf.get(Property.INSTANCE_SECRET), true);
+ }
+
+ public static ZooReaderWriter retriesDisabled(AccumuloConfiguration conf) {
+ return new ZooReaderWriter(conf.get(Property.INSTANCE_ZK_HOST),
+ (int) conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
+ conf.get(Property.INSTANCE_SECRET), false);
+ }
+
+ public static ZooReaderWriter retriesEnabled(String keepers, int timeoutInMillis, String secret) {
+ return new ZooReaderWriter(keepers, timeoutInMillis, secret, true);
+ }
+
+ public static ZooReaderWriter retriesDisabled(String keepers, int timeoutInMillis,
+ String secret) {
+ return new ZooReaderWriter(keepers, timeoutInMillis, secret, false);
}
private final byte[] auth;
- public ZooReaderWriter(String keepers, int timeoutInMillis, String secret) {
- super(keepers, timeoutInMillis);
+ private ZooReaderWriter(String keepers, int timeoutInMillis, String secret,
+ boolean enableRetries) {
+ super(keepers, timeoutInMillis, enableRetries);
Review comment:
Could pass in the retry factory rather than pass a boolean around. That would make the code more clear.
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
##########
@@ -1384,4 +1385,14 @@ public final RateLimiter getMajorCompactionWriteLimiter() {
public CompactionManager getCompactionManager() {
return compactionManager;
}
+
+ /**
+ * For the tablet server we don't want ZooReaderWriter to keep retrying after a error
+ * communicating with ZooKeeper.
+ */
+ @Override
+ protected ServerContext createServerContext(SiteConfiguration siteConfig) {
+ return new ServerContext(siteConfig, false);
+ }
+
Review comment:
I worry that this will have a larger impact on the various ways ZooKeeper is used in a TabletServer, rather than be a narrow fix for ZooLock. The ServerContext is universal for the process. The only ZooReaderWriter instance for which we want to avoid retries is the one used by ZooLock. Is there a reason we need to change the behavior across the entire tserver?
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java
##########
@@ -38,24 +38,34 @@
public class ZooReader {
private static final Logger log = LoggerFactory.getLogger(ZooReader.class);
- protected static final RetryFactory RETRY_FACTORY = Retry.builder().maxRetries(10)
+ private 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();
+
+ private 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();
protected final String keepers;
protected final int timeout;
+ protected final RetryFactory retryFactory;
public ZooReader(String keepers, int timeout) {
+ this(keepers, timeout, true);
+ }
+
+ public ZooReader(String keepers, int timeout, boolean enableRetries) {
Review comment:
Could pass in the RetryFactory rather than have a boolean param.
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
Review comment:
Could pass in the retry factory rather than pass around booleans.
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
This worries me. It seems outside the scope of ServerContext to be handling this kind of thing. ServerContext should merely be a holder of the ZRW that is available to the server for updating/reading from ZK. The affected code is in ZooLock, and is not a problem with ZooReaderWriter's retry behavior more generally.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion closed pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion closed pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469943207
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
> It sounds as if you both might be leaning toward a solution that does not use ZooLock for the tablet server lock, but uses something else that does not retry and communicates directly with ZooKeeper. Is that correct?
I don't think that's quite right. I don't think it makes sense to distance ourselves from using ZooLock for locking (doesn't it exist for that purpose?). Rather, I think we're suggesting that the implementation of ZooLock should be changed to distance itself from the general-purpose utilities of ZooReaderWriter/ZooSession. This can be done by making ZooLock never use those at all and use something of its own, or as Keith suggested, adding special-purpose methods on those utilities for ZooLock to safely use.
> @ctubbsii - did you want me to wait for #1459 to be completed before revisiting this?
That depends. #1459 had no intention of touching ZooLock, so if you want to try to make ZooLock work without ZooReaderWriter/ZooSession, then your efforts should not conflict with that and can proceed independently. If, however, you want to try to add special-purpose methods for ZooLock on ZooReaderWriter/ZooSession, then it should wait, and I can prioritize my efforts next week to clean those up to make it easier for you.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-717632990
I have completed a refactor of some of the Zoo-related utilities in #1755 . After that PR is merged, it might be easier to identify alternate ways to address this. (Or, it may not be any help... I'm not sure. It's been awhile since I looked at this issue.)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r470236273
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
@dlmarion I was thinking of looking into making ZooLock use Zookeeper directly to see if that is workable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469979823
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
> Rather, I think we're suggesting that the implementation of ZooLock should be changed to distance itself from the general-purpose utilities of ZooReaderWriter/ZooSession. This can be done by making ZooLock never use those at all and use something of its own, or as Keith suggested, adding special-purpose methods on those utilities for ZooLock to safely use.
Understood, I'll work down that path. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1677: fixes #1086 - Disable retries from the server components
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469212023
##########
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:
Looking at the interfaces they are chained together such that they are needed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469926930
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
Thanks for the feedback on the PR. I read back through the comments on #1086 and the comments above. My previous attempts were to create a code path through the existing objects that didn't retry instead of creating something new that also talks with ZK. It sounds as if you both might be leaning toward a solution that does not use ZooLock for the tablet server lock, but uses something else that does not retry and communicates directly with ZooKeeper. Is that correct?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-767789538
I delete the previous PR and branch as it's being addressed [here](https://github.com/dlmarion/accumulo/pull/3). It looks like `automation` marked it `Done`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469935334
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
@ctubbsii - did you want me to wait for #1459 to be completed before revisiting this?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1677: fixes #1086 - Disable retries from the server components
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #1677: fixes #1086 - Disable retries from the server components
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-672881754
FYI the original ticket #1086 was for the 1.9 branch which will probably be a very different fix. We may chose to not fix it in 1.10 but just wanted to give a heads up that this could auto close that ticket.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469633898
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
If we do something like add a retry to every method, I'd like to revisit my effort to clean up the ZooReaderWriter stuff, like what I had started to do in #1459 ; it would be easier to add something like a retry to every method, if the code were cleaned up a bit first. I can revisit my previous cleanup effort over the next week.
In the meantime, I still think it's probably best to simply avoid the problematic ZRW code in the ZooLock. ZooLock does not need to use the general-purpose utility code, when it can use something specific to its needs.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion edited a comment on pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion edited a comment on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-767789538
I deleted the previous PR and branch as it's being addressed [here](https://github.com/dlmarion/accumulo/pull/3). It looks like `automation` marked it `Done`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#discussion_r469603172
##########
File path: server/base/src/main/java/org/apache/accumulo/server/ServerContext.java
##########
@@ -68,13 +68,22 @@
private CryptoService cryptoService = null;
public ServerContext(SiteConfiguration siteConfig) {
- this(new ServerInfo(siteConfig));
+ this(new ServerInfo(siteConfig), true);
+ }
+
+ public ServerContext(SiteConfiguration siteConfig, boolean enableZookeeperRetries) {
+ this(new ServerInfo(siteConfig), enableZookeeperRetries);
}
private ServerContext(ServerInfo info) {
+ this(info, true);
+ }
+
+ private ServerContext(ServerInfo info, boolean enableRetries) {
super(SingletonReservation.noop(), info, info.getSiteConfiguration());
this.info = info;
- zooReaderWriter = new ZooReaderWriter(info.getSiteConfiguration());
+ zooReaderWriter = enableRetries ? ZooReaderWriter.retriesEnabled(info.getSiteConfiguration())
+ : ZooReaderWriter.retriesDisabled(info.getSiteConfiguration());
Review comment:
I am not sure this a good idea, but its something I thought of after reading this comment. Could make every ZooReaderWriter method accept a retry factory and have overloaded versions that do not take a retry factory and use a default. This seems tedious though, but its the best I could think of so far to associate the retry logic with method calls instead of the object.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-767770867
@dlmarion This was closed as marked as "Done" for 2.1.0, but I don't think it was merged. I think it's being addressed via a different approach. Is that correct?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1677: fixes #1086 - Disable Zookeeper retries from TabletServer
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1677:
URL: https://github.com/apache/accumulo/pull/1677#issuecomment-672962112
@milleruntime - good catch. I created another PR for the 1.9 branch at https://github.com/apache/accumulo/pull/1679
----------------------------------------------------------------
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:
users@infra.apache.org