[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154476081
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
executeVoidAdmin is ok with me
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154458309
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
ok I agree. I was thinking only the admin task would call `executeVoid(ctx,
exec, false)` everything else would continue to call the `executeVoid(ctx,
exec)` that retries.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154458309
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
ok I agree. I was thinking only the admin task would call
`executeVoid(exec, false)` everything else would continue to call the
`executeVoid(exec)` that retries.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154458309
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
ok I agree. I was thinking only the admin task would call `executeVoid(...,
false)`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154384723
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
> What if we have some networking issue ? This way we cannot recover...
Can you give me an example of the situation you are thinking of?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r154186590
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
I am thinking we may want to provide a way to not retry at all. I don't
think retrying twice vs once offers much benefit. If it fails the first time
it will likely fail the second if retrying immediately.
I have been looking at the code and there is a retry loop in the
`executeGeneric()` in addition to the retry loop in `getConnectionWithRetry()`.
I am thinking this code should call an `executeVoid()` method that has no
retry loops. Maybe something like the following.
```java
public static void executeVoid(ClientContext context,
ClientExec exec, boolean retry)
throws AccumuloException, AccumuloSecurityException {
if (retry) {
executeVoid(context, exec);
} else {
MasterClientService.Client client = null;
try {
// TODO this gets a connection without a timeout
client = getConnection(context);
if (client == null) {
throw new AccumuloException("Failed to connect to master " +
context.getInstance().getMasterLocations());
}
exec.execute(client);
} catch (ThriftSecurityException e) {
throw new AccumuloSecurityException(e.user, e.code, e);
} catch (AccumuloException e) {
throw e;
} catch (RuntimeException e) {
throw e;
} catch (Exception e) {
throw new AccumuloException(e);
} finally {
if (client != null)
close(client);
}
}
}
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r153328920
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
I suggested making this time based instead of count based in another
comment. That is why I was thinking of that name. A more general name could
be `executeVoidWithLimitedRetries()`. This name implies it will not retry
forever, but does not specify how its limited. It could be limited by count or
time.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r153278211
##
File path:
core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java
##
@@ -41,12 +41,19 @@
public class MasterClient {
private static final Logger log =
LoggerFactory.getLogger(MasterClient.class);
- public static MasterClientService.Client
getConnectionWithRetry(ClientContext context) {
+ public static MasterClientService.Client
getConnectionWithRetry(ClientContext context, int numberOfRetries) {
Review comment:
Instead of passing a count, could pass a max time. I think passing a time
assumes less about the implementation. Could do something like the following.
```java
public static MasterClientService.Client
getConnectionWithRetry(ClientContext context, long maxRetryTime, TimeUnit
retryTimeUnit) {
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r153277305
##
File path:
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
##
@@ -244,7 +244,7 @@ private long beginFateOperation() throws
ThriftSecurityException, TException {
while (true) {
MasterClientService.Iface client = null;
try {
-client = MasterClient.getConnectionWithRetry(context);
+client = MasterClient.getConnectionWithRetry(context, 0);
Review comment:
Could have a method `getConnectionWithRetry(ctx)` without the count that
default to 0/infinite retires. Then all of these calls do not need to pass
zero.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] keith-turner commented on a change in pull request #325: ACCUMULO-2341?
keith-turner commented on a change in pull request #325: ACCUMULO-2341?
URL: https://github.com/apache/accumulo/pull/325#discussion_r153279237
##
File path: server/base/src/main/java/org/apache/accumulo/server/util/Admin.java
##
@@ -354,12 +354,8 @@ public void run() {
}
private static void stopServer(final ClientContext context, final boolean
tabletServersToo) throws AccumuloException, AccumuloSecurityException {
-MasterClient.executeVoid(context, new
ClientExec() {
- @Override
- public void execute(MasterClientService.Client client) throws Exception {
-client.shutdown(Tracer.traceInfo(), context.rpcCreds(),
tabletServersToo);
- }
-});
+MasterClient.executeVoidWithConnRetry(context,
Review comment:
Is this the only place in the code that is timing out now? I am wondering
this method should have a different name, like `executeVoidWithTimeout()`. If
I am looking at this code, its not clear that this call will timeout.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
