You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@accumulo.apache.org by ct...@apache.org on 2019/06/14 23:47:08 UTC
[accumulo] branch master updated: Revert "De-dupe some code (#1211)"
This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/master by this push:
new ca5b62d Revert "De-dupe some code (#1211)"
ca5b62d is described below
commit ca5b62d81b21a35bb260f1c5d3257811eeb9ae6e
Author: Christopher Tubbs <ct...@apache.org>
AuthorDate: Fri Jun 14 19:41:33 2019 -0400
Revert "De-dupe some code (#1211)"
This reverts commit 417de4e6da1f03c18e3717f2c359828f500197b2.
This is reverted because it breaks two tests in ZooKeeperInstanceTest,
one of which can be updated to test the equivalent scenario in ZooUtil,
but the other test breakage reveals that this change fails to properly
handle the the lookup when the ZooKeeperInstance is constructed with the
instanceId (instanceName is null) and the instanceId does not correspond
to a valid location in ZooKeeper. This situation should result in a
RuntimeException, but it does not.
---
.../accumulo/core/client/ZooKeeperInstance.java | 46 +++++++++++++++++++++-
.../accumulo/core/clientImpl/ClientContext.java | 46 +++++++++++++++++++++-
.../core/clientImpl/InstanceOperationsImpl.java | 1 +
.../apache/accumulo/fate/zookeeper/ZooUtil.java | 46 ----------------------
4 files changed, 89 insertions(+), 50 deletions(-)
diff --git a/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java b/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
index b61e4f2..9b3bd24 100644
--- a/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
+++ b/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
@@ -17,12 +17,15 @@
package org.apache.accumulo.core.client;
import static com.google.common.base.Preconditions.checkArgument;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import java.util.Collections;
import java.util.List;
import java.util.Properties;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
+import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.client.security.tokens.AuthenticationToken;
import org.apache.accumulo.core.clientImpl.ClientConfConverter;
import org.apache.accumulo.core.clientImpl.ClientContext;
@@ -122,14 +125,53 @@ public class ZooKeeperInstance implements Instance {
@Override
public String getInstanceID() {
if (instanceId == null) {
- instanceId = ZooUtil.getInstanceID(zooCache, instanceName);
+ // want the instance id to be stable for the life of this instance object,
+ // so only get it once
+ String instanceNamePath = Constants.ZROOT + Constants.ZINSTANCES + "/" + instanceName;
+ byte[] iidb = zooCache.get(instanceNamePath);
+ if (iidb == null) {
+ throw new RuntimeException(
+ "Instance name " + instanceName + " does not exist in zookeeper. "
+ + "Run \"accumulo org.apache.accumulo.server.util.ListInstances\" to see a list.");
+ }
+ instanceId = new String(iidb, UTF_8);
}
+
+ if (zooCache.get(Constants.ZROOT + "/" + instanceId) == null) {
+ if (instanceName == null)
+ throw new RuntimeException("Instance id " + instanceId + " does not exist in zookeeper");
+ throw new RuntimeException("Instance id " + instanceId + " pointed to by the name "
+ + instanceName + " does not exist in zookeeper");
+ }
+
return instanceId;
}
@Override
public List<String> getMasterLocations() {
- return ZooUtil.getMasterLocations(zooCache, getInstanceID());
+ String masterLocPath = ZooUtil.getRoot(getInstanceID()) + Constants.ZMASTER_LOCK;
+
+ OpTimer timer = null;
+
+ if (log.isTraceEnabled()) {
+ log.trace("tid={} Looking up master location in zookeeper.", Thread.currentThread().getId());
+ timer = new OpTimer().start();
+ }
+
+ byte[] loc = ZooUtil.getLockData(zooCache, masterLocPath);
+
+ if (timer != null) {
+ timer.stop();
+ log.trace("tid={} Found master at {} in {}", Thread.currentThread().getId(),
+ (loc == null ? "null" : new String(loc, UTF_8)),
+ String.format("%.3f secs", timer.scale(TimeUnit.SECONDS)));
+ }
+
+ if (loc == null) {
+ return Collections.emptyList();
+ }
+
+ return Collections.singletonList(new String(loc, UTF_8));
}
@Override
diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
index 0737aa2..fa8b73e 100644
--- a/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
+++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
@@ -17,9 +17,11 @@
package org.apache.accumulo.core.clientImpl;
import static com.google.common.base.Preconditions.checkArgument;
+import static java.nio.charset.StandardCharsets.UTF_8;
import static org.apache.accumulo.core.metadata.schema.TabletMetadata.ColumnType.LOCATION;
import java.nio.file.Path;
+import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.Properties;
@@ -27,6 +29,7 @@ import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import java.util.function.Supplier;
+import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.client.AccumuloClient;
import org.apache.accumulo.core.client.AccumuloException;
import org.apache.accumulo.core.client.AccumuloSecurityException;
@@ -363,7 +366,29 @@ public class ClientContext implements AccumuloClient {
*/
public List<String> getMasterLocations() {
ensureOpen();
- return ZooUtil.getMasterLocations(zooCache, instanceId);
+ String masterLocPath = getZooKeeperRoot() + Constants.ZMASTER_LOCK;
+
+ OpTimer timer = null;
+
+ if (log.isTraceEnabled()) {
+ log.trace("tid={} Looking up master location in zookeeper.", Thread.currentThread().getId());
+ timer = new OpTimer().start();
+ }
+
+ byte[] loc = ZooUtil.getLockData(zooCache, masterLocPath);
+
+ if (timer != null) {
+ timer.stop();
+ log.trace("tid={} Found master at {} in {}", Thread.currentThread().getId(),
+ (loc == null ? "null" : new String(loc, UTF_8)),
+ String.format("%.3f secs", timer.scale(TimeUnit.SECONDS)));
+ }
+
+ if (loc == null) {
+ return Collections.emptyList();
+ }
+
+ return Collections.singletonList(new String(loc, UTF_8));
}
/**
@@ -375,8 +400,25 @@ public class ClientContext implements AccumuloClient {
ensureOpen();
final String instanceName = info.getInstanceName();
if (instanceId == null) {
- instanceId = ZooUtil.getInstanceID(zooCache, instanceName);
+ // want the instance id to be stable for the life of this instance object,
+ // so only get it once
+ String instanceNamePath = Constants.ZROOT + Constants.ZINSTANCES + "/" + instanceName;
+ byte[] iidb = zooCache.get(instanceNamePath);
+ if (iidb == null) {
+ throw new RuntimeException(
+ "Instance name " + instanceName + " does not exist in zookeeper. "
+ + "Run \"accumulo org.apache.accumulo.server.util.ListInstances\" to see a list.");
+ }
+ instanceId = new String(iidb, UTF_8);
+ }
+
+ if (zooCache.get(Constants.ZROOT + "/" + instanceId) == null) {
+ if (instanceName == null)
+ throw new RuntimeException("Instance id " + instanceId + " does not exist in zookeeper");
+ throw new RuntimeException("Instance id " + instanceId + " pointed to by the name "
+ + instanceName + " does not exist in zookeeper");
}
+
return instanceId;
}
diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java
index 79411cb..a452ce4 100644
--- a/core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java
+++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java
@@ -235,6 +235,7 @@ public class InstanceOperationsImpl implements InstanceOperations {
@Override
public String getInstanceID() {
+
return context.getInstanceID();
}
}
diff --git a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java
index fc92ac8..6573614 100644
--- a/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java
+++ b/core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooUtil.java
@@ -16,7 +16,6 @@
*/
package org.apache.accumulo.fate.zookeeper;
-import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
@@ -32,7 +31,6 @@ import java.util.concurrent.TimeUnit;
import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.conf.AccumuloConfiguration;
-import org.apache.accumulo.core.util.OpTimer;
import org.apache.accumulo.core.volume.VolumeConfiguration;
import org.apache.accumulo.fate.util.Retry;
import org.apache.accumulo.fate.util.Retry.RetryFactory;
@@ -616,48 +614,4 @@ public class ZooUtil {
}
}
- public static String getInstanceID(ZooCache zooCache, String instanceName) {
- String instanceId;
- String instanceNamePath = Constants.ZROOT + Constants.ZINSTANCES + "/" + instanceName;
- byte[] data = zooCache.get(instanceNamePath);
- if (data == null) {
- throw new RuntimeException("Instance name " + instanceName + " does not exist in zookeeper. "
- + "Run \"accumulo org.apache.accumulo.server.util.ListInstances\" to see a list.");
- }
- instanceId = new String(data, UTF_8);
-
- if (zooCache.get(Constants.ZROOT + "/" + instanceId) == null) {
- if (instanceName == null)
- throw new RuntimeException("Instance id " + instanceId + " does not exist in zookeeper");
- throw new RuntimeException("Instance id " + instanceId + " pointed to by the name "
- + instanceName + " does not exist in zookeeper");
- }
- return instanceId;
- }
-
- public static List<String> getMasterLocations(ZooCache zooCache, String instanceId) {
- String masterLocPath = ZooUtil.getRoot(instanceId) + Constants.ZMASTER_LOCK;
-
- OpTimer timer = null;
-
- if (log.isTraceEnabled()) {
- log.trace("tid={} Looking up master location in zookeeper.", Thread.currentThread().getId());
- timer = new OpTimer().start();
- }
-
- byte[] loc = ZooUtil.getLockData(zooCache, masterLocPath);
-
- if (timer != null) {
- timer.stop();
- log.trace("tid={} Found master at {} in {}", Thread.currentThread().getId(),
- (loc == null ? "null" : new String(loc, UTF_8)),
- String.format("%.3f secs", timer.scale(TimeUnit.SECONDS)));
- }
-
- if (loc == null) {
- return Collections.emptyList();
- }
-
- return Collections.singletonList(new String(loc, UTF_8));
- }
}