kevinrr888 commented on code in PR #5350:
URL: https://github.com/apache/accumulo/pull/5350#discussion_r1968053486
##########
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java:
##########
@@ -79,40 +79,29 @@ void initializeConfig(final InstanceId instanceId, final
ZooReaderWriter zoo) {
if (zoo.exists(sysPropPath)) {
return;
}
- var created = zoo.putPrivatePersistentData(sysPropPath,
+ var created = zoo.putPrivatePersistentData(zkInstanceRoot + sysPropPath,
VersionedPropCodec.getDefault().toBytes(vProps),
ZooUtil.NodeExistsPolicy.FAIL);
if (!created) {
throw new IllegalStateException(
"Failed to create default system props during initialization at:
{}" + sysPropPath);
}
+ // if (!zoo.exists("/yea")) {
+ // throw new IllegalStateException("heres the stuff under
root/iid/config: "
+ // + Arrays.toString(zoo.getData(zkInstanceRoot + Constants.ZCONFIG)));
+ // }
Review Comment:
some debugging that was accidentally left in
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -534,12 +534,10 @@ static TabletMetadata create(String id, String
prevEndRow, String endRow) {
public static synchronized Set<TServerInstance>
getLiveTServers(ClientContext context) {
final Set<TServerInstance> liveServers = new HashSet<>();
- final String path = context.getZooKeeperRoot() + Constants.ZTSERVERS;
-
- for (String child : context.getZooCache().getChildren(path)) {
- checkServer(context, path, child).ifPresent(liveServers::add);
+ for (String child :
context.getZooCache().getChildren(Constants.ZTSERVERS)) {
+ checkServer(context, Constants.ZTSERVERS,
child).ifPresent(liveServers::add);
}
- log.trace("Found {} live tservers at ZK path: {}", liveServers.size(),
path);
+ log.trace("Found {} live tservers at ZK path: {}", liveServers.size(),
Constants.ZTSERVERS);
Review Comment:
We would probably still want to log the full path
##########
server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java:
##########
@@ -205,4 +193,20 @@ public void initScanRefTableState(ServerContext context) {
}
}
+ public void initInstanceNameAndId(ZooReaderWriter zoo, InstanceId instanceId,
Review Comment:
Is this called in a different class? I don't see this called in
`ZooKeeperInitializer.initialize()` where I thought it would be called
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -341,11 +341,11 @@ private ProcessInfo _exec(Class<?> clazz, List<String>
extraJvmOpts, String... a
// @formatter:off
var hardcodedArgs = Stream.of(
- "-Dapple.awt.UIElement=true",
- "-Djava.net.preferIPv4Stack=true",
- "-XX:+PerfDisableSharedMem",
- "-XX:+AlwaysPreTouch",
- Main.class.getName(), clazz.getName());
+ "-Dapple.awt.UIElement=true",
+ "-Djava.net.preferIPv4Stack=true",
+ "-XX:+PerfDisableSharedMem",
+ "-XX:+AlwaysPreTouch",
+ Main.class.getName(), clazz.getName());
Review Comment:
Was there a reason for this change? Looks like an auto formatter change but
the formatter is turned off above which is strange. I think this can be changed
back
##########
server/base/src/main/java/org/apache/accumulo/server/util/TabletServerLocks.java:
##########
@@ -33,19 +33,18 @@ public class TabletServerLocks {
public static void execute(final ServerContext context, final String lock,
final String delete)
throws Exception {
- String tserverPath = context.getZooKeeperRoot() + Constants.ZTSERVERS;
ZooCache cache = context.getZooCache();
ZooReaderWriter zoo = context.getZooSession().asReaderWriter();
if (delete == null) {
- List<String> tabletServers = zoo.getChildren(tserverPath);
+ List<String> tabletServers = zoo.getChildren(Constants.ZTSERVERS);
if (tabletServers.isEmpty()) {
- System.err.println("No tservers found in ZK at " + tserverPath);
+ System.err.println("No tservers found in ZK at " +
Constants.ZTSERVERS);
Review Comment:
Probably still want to see the full path
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServiceStatusCmd.java:
##########
@@ -59,19 +59,24 @@ public void execute(final ServerContext context, final
boolean json, final boole
ZooReader zooReader = context.getZooSession().asReader();
- final String zooRoot = context.getZooKeeperRoot();
- LOG.trace("zooRoot: {}", zooRoot);
+ LOG.trace("zooRoot: {}", "");
Review Comment:
This log should be updated or removed
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java:
##########
@@ -210,7 +210,7 @@ protected void getCoordinatorLock(HostAndPort clientAddress)
LOG.info("trying to get coordinator lock");
final String coordinatorClientAddress =
ExternalCompactionUtil.getHostPortString(clientAddress);
- final String lockPath = getContext().getZooKeeperRoot() +
Constants.ZCOORDINATOR_LOCK;
+ final String lockPath = Constants.ZCOORDINATOR_LOCK;
Review Comment:
can be inlined
##########
server/compaction-coordinator/src/main/java/org/apache/accumulo/coordinator/CompactionCoordinator.java:
##########
@@ -750,7 +750,7 @@ private void deleteEmpty(ZooReaderWriter zoorw, String path)
}
private void cleanUpCompactors() {
- final String compactorQueuesPath = getContext().getZooKeeperRoot() +
Constants.ZCOMPACTORS;
+ final String compactorQueuesPath = Constants.ZCOMPACTORS;
Review Comment:
can be inlined
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/create/PopulateZookeeperWithNamespace.java:
##########
@@ -53,9 +53,8 @@ public Repo<Manager> call(long tid, Manager manager) throws
Exception {
Utils.getTableNameLock().lock();
try {
var context = manager.getContext();
- NamespaceMapping.put(context.getZooSession().asReaderWriter(),
- context.getZooKeeperRoot() + Constants.ZNAMESPACES,
namespaceInfo.namespaceId,
- namespaceInfo.namespaceName);
+ NamespaceMapping.put(context.getZooSession().asReaderWriter(),
Constants.ZNAMESPACES,
+ namespaceInfo.namespaceId, namespaceInfo.namespaceName);
Review Comment:
Should probably avoid passing a constant
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthorizor.java:
##########
@@ -41,16 +42,15 @@ public class ZKAuthorizor implements Authorizor {
private static final Logger log =
LoggerFactory.getLogger(ZKAuthorizor.class);
private final String ZKUserAuths = "/Authorizations";
+ private final String zkUserPath = Constants.ZUSERS;
Review Comment:
Same comment. Could also do the same for ZKUserAuths but that should
probably be static not inlined
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKPermHandler.java:
##########
@@ -54,21 +54,16 @@ public class ZKPermHandler implements PermissionHandler {
private static final Logger log =
LoggerFactory.getLogger(ZKPermHandler.class);
private ZooReaderWriter zoo;
- private String zkUserPath;
- private String ZKTablePath;
- private String ZKNamespacePath;
private ZooCache zooCache;
private final String ZKUserSysPerms = "/System";
private final String ZKUserTablePerms = "/Tables";
private final String ZKUserNamespacePerms = "/Namespaces";
+ private final String zkUserPath = Constants.ZUSERS;
Review Comment:
These could all be changed to static. Can inline zkUserPath
##########
server/base/src/test/java/org/apache/accumulo/server/security/delegation/ZooAuthenticationKeyWatcherTest.java:
##########
@@ -70,7 +69,7 @@ public static void setupKeyGenerator() throws Exception {
private ZooSession zk;
private InstanceId instanceId;
- private String baseNode;
+ private final String baseNode = Constants.ZDELEGATION_TOKEN_KEYS;
Review Comment:
can be static or inlined
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServiceStatusCmd.java:
##########
@@ -59,19 +59,24 @@ public void execute(final ServerContext context, final
boolean json, final boole
ZooReader zooReader = context.getZooSession().asReader();
- final String zooRoot = context.getZooKeeperRoot();
- LOG.trace("zooRoot: {}", zooRoot);
+ LOG.trace("zooRoot: {}", "");
final Map<ServiceStatusReport.ReportKey,StatusSummary> services = new
TreeMap<>();
- services.put(ServiceStatusReport.ReportKey.MANAGER,
getManagerStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.MONITOR,
getMonitorStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.T_SERVER,
getTServerStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.S_SERVER,
getScanServerStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.COORDINATOR,
- getCoordinatorStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.COMPACTOR,
getCompactorStatus(zooReader, zooRoot));
- services.put(ServiceStatusReport.ReportKey.GC, getGcStatus(zooReader,
zooRoot));
+ services.put(ServiceStatusReport.ReportKey.MANAGER, getStatusSummary(
+ ServiceStatusReport.ReportKey.MANAGER, zooReader,
Constants.ZMANAGER_LOCK));
+ services.put(ServiceStatusReport.ReportKey.MONITOR, getStatusSummary(
+ ServiceStatusReport.ReportKey.MONITOR, zooReader,
Constants.ZMONITOR_LOCK));
+ services.put(ServiceStatusReport.ReportKey.T_SERVER,
getServerHostStatus(zooReader,
+ Constants.ZTSERVERS, ServiceStatusReport.ReportKey.T_SERVER, TSERV));
+ services.put(ServiceStatusReport.ReportKey.S_SERVER,
getServerHostStatus(zooReader,
+ Constants.ZSSERVERS, ServiceStatusReport.ReportKey.S_SERVER,
TABLET_SCAN));
+ services.put(ServiceStatusReport.ReportKey.COORDINATOR, getStatusSummary(
+ ServiceStatusReport.ReportKey.COORDINATOR, zooReader,
Constants.ZCOORDINATOR_LOCK));
+ services.put(ServiceStatusReport.ReportKey.COMPACTOR,
+ getCompactorHosts(zooReader, Constants.ZCOMPACTORS));
+ services.put(ServiceStatusReport.ReportKey.GC,
+ getStatusSummary(ServiceStatusReport.ReportKey.GC, zooReader,
Constants.ZGC_LOCK));
Review Comment:
This is a nice improvement combining all those methods into one. Could push
the Constant that is passed as an arg to instead be computed in the method.
Could just add a switch which computes where the lock is... MANAGER ->
ZMANAGER_LOCK, MONITOR -> ZMONITOR_LOCK etc. Similar thing for
`getCompactorHosts` and `getServerHostStatus`
##########
server/manager/src/main/java/org/apache/accumulo/manager/upgrade/PreUpgradeValidation.java:
##########
@@ -88,7 +88,8 @@ private void validateACLs(ServerContext context) {
+ "for instructions on how to fix.");
}
} catch (KeeperException | InterruptedException e) {
- throw new RuntimeException("Upgrade Failed! Error validating nodes under
" + rootPath, e);
+ throw new RuntimeException("Upgrade Failed! Error validating nodes under
" + Constants.ZROOT,
+ e);
Review Comment:
ZROOT = /accumulo
Is this the correct error message we want here? Or should it be specific to
the path to the instance
##########
server/base/src/main/java/org/apache/accumulo/server/security/handler/ZKAuthenticator.java:
##########
@@ -41,16 +42,15 @@
// Utility class for adding all authentication info into ZK
public final class ZKAuthenticator implements Authenticator {
private static final Logger log =
LoggerFactory.getLogger(ZKAuthenticator.class);
+ private final String zkUserPath = Constants.ZUSERS;
Review Comment:
This could be static or better yet just inlined everywhere it is used in
this class
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/DeadServerList.java:
##########
@@ -38,29 +38,27 @@ public class DeadServerList {
private static final Logger log =
LoggerFactory.getLogger(DeadServerList.class);
- private final String path;
private final ZooReaderWriter zoo;
public DeadServerList(ServerContext context) {
- this.path = context.getZooKeeperRoot() + Constants.ZDEADTSERVERS;
zoo = context.getZooSession().asReaderWriter();
try {
- zoo.mkdirs(path);
+ zoo.mkdirs(Constants.ZDEADTSERVERS);
} catch (Exception ex) {
- log.error("Unable to make parent directories of " + path, ex);
+ log.error("Unable to make parent directories of " +
Constants.ZDEADTSERVERS, ex);
Review Comment:
another place where we might still want the full path to be logged
##########
server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java:
##########
@@ -543,7 +543,7 @@ public void execute(final String[] args) {
success = doInit(zk.asReaderWriter(), opts, fs, initConfig);
}
}
- } catch (IOException e) {
+ } catch (IOException | InterruptedException | KeeperException e) {
log.error("Problem trying to get Volume configuration", e);
success = false;
Review Comment:
It seems like new catch blocks should be added for these new exceptions from
`doInit`
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -681,9 +679,8 @@ private void verifyUp() throws InterruptedException,
IOException {
try {
while (tsActualCount < tsExpectedCount) {
tsActualCount = 0;
- String tserverPath = rootPath + Constants.ZTSERVERS;
- for (String child : rdr.getChildren(tserverPath)) {
- if (rdr.getChildren(tserverPath + "/" + child).isEmpty()) {
+ for (String child : rdr.getChildren(rootPath + Constants.ZTSERVERS))
{
+ if (rdr.getChildren(rootPath + Constants.ZTSERVERS + "/" +
child).isEmpty()) {
Review Comment:
This section can be changed to what it was previously. Nicer to have the var
here
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -1297,20 +1293,20 @@ public void run() {
ZooReaderWriter zReaderWriter = context.getZooSession().asReaderWriter();
try {
- zReaderWriter.getChildren(zroot + Constants.ZRECOVERY, new Watcher() {
+ zReaderWriter.getChildren(Constants.ZRECOVERY, new Watcher() {
@Override
public void process(WatchedEvent event) {
nextEvent.event("Noticed recovery changes %s", event.getType());
try {
// watcher only fires once, add it back
- zReaderWriter.getChildren(zroot + Constants.ZRECOVERY, this);
+ zReaderWriter.getChildren(Constants.ZRECOVERY, this);
} catch (Exception e) {
log.error("Failed to add log recovery watcher back", e);
}
}
});
} catch (KeeperException | InterruptedException e) {
- throw new IllegalStateException("Unable to read " + zroot +
Constants.ZRECOVERY, e);
+ throw new IllegalStateException("Unable to read " + Constants.ZRECOVERY,
e);
Review Comment:
Probably want full path here
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/rename/RenameNamespace.java:
##########
@@ -56,8 +56,7 @@ public Repo<Manager> call(long id, Manager manager) throws
Exception {
Utils.getTableNameLock().lock();
try {
- NamespaceMapping.rename(zoo, manager.getContext().getZooKeeperRoot() +
Constants.ZNAMESPACES,
- namespaceId, oldName, newName);
+ NamespaceMapping.rename(zoo, Constants.ZNAMESPACES, namespaceId,
oldName, newName);
Review Comment:
Should probably avoid passing a constant
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
