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 2021/02/05 16:20:24 UTC
[GitHub] [accumulo] brianloss opened a new pull request #1912: Rename master comment/constant/variable references.
brianloss opened a new pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912
This change renames reference to master in comments, variable names,
constants, etc. to manager. In some deprecated classes whose purpose is
to maintain compatibility until the next major release, the references
were not renamed.
fixes #1642
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572365151
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
Review comment:
Would checking if the master dir exists instead of the manager dir does not exists make this operation idempotent?
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572365151
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
Review comment:
Would checking if the master dir exists instead of if the manager dir does not exists make this operation idempotent?
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572380347
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
Review comment:
@keith-turner I pushed the changes--have a look and let me know if I missed anything. 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] milleruntime commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572092741
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -42,11 +42,11 @@
public static final String ZNAMESPACE_NAME = "/name";
public static final String ZNAMESPACE_CONF = "/conf";
- public static final String ZMASTERS = "/masters";
- public static final String ZMASTER_LOCK = ZMASTERS + "/lock";
- public static final String ZMASTER_GOAL_STATE = ZMASTERS + "/goal_state";
- public static final String ZMASTER_REPLICATION_COORDINATOR_ADDR = ZMASTERS + "/repl_coord_addr";
- public static final String ZMASTER_TICK = ZMASTERS + "/tick";
+ public static final String ZMANAGERS = "/masters";
Review comment:
`SetGoalState` isn't a bad idea since I think it has to always run first. It might be worth it to put the Upgrade code in its own separate utility that an Admin has the option to manually call.
----------------------------------------------------------------
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] brianloss merged pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss merged pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572260732
##########
File path: server/base/src/main/java/org/apache/accumulo/server/log/WalStateManager.java
##########
@@ -44,7 +44,7 @@
* by tablet servers and the replication machinery.
*
* <p>
- * The Master needs to know the state of the WALs to mark tablets during recovery. The GC needs to
+ * The Manager needs to know the state of the WALs to mark tablets during recovery. The GC needs to
Review comment:
Given this class is called WalStateManager, use of Manager in the docs may be a bit ambiguous. Thinking could do the following, but not sure its needed.
```suggestion
* The Accumulo Manager needs to know the state of the WALs to mark tablets during recovery. The GC needs to
```
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
Review comment:
Will this always show up in the console when starting Accumulo? If so maybe it could be debug.
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
+ if (managersDirMissing) {
+ LOG.info("Renaming ZooKeeper directory {} to {}.", mastersZooDir, managersZooDir);
+ zoo.recursiveCopyPersistentOverwrite(mastersZooDir, managersZooDir);
+ zoo.recursiveDelete(mastersZooDir, ZooUtil.NodeMissingPolicy.SKIP);
Review comment:
Given how important this is, could be more verbose.
```suggestion
LOG.info("Copying ZooKeeper directory {} to {}.", mastersZooDir, managersZooDir);
zoo.recursiveCopyPersistentOverwrite(mastersZooDir, managersZooDir);
LOG.info("Deleting ZooKeeper directory {}.", mastersZooDir);
zoo.recursiveDelete(mastersZooDir, ZooUtil.NodeMissingPolicy.SKIP);
```
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r571087563
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -42,11 +42,11 @@
public static final String ZNAMESPACE_NAME = "/name";
public static final String ZNAMESPACE_CONF = "/conf";
- public static final String ZMASTERS = "/masters";
- public static final String ZMASTER_LOCK = ZMASTERS + "/lock";
- public static final String ZMASTER_GOAL_STATE = ZMASTERS + "/goal_state";
- public static final String ZMASTER_REPLICATION_COORDINATOR_ADDR = ZMASTERS + "/repl_coord_addr";
- public static final String ZMASTER_TICK = ZMASTERS + "/tick";
+ public static final String ZMANAGERS = "/masters";
Review comment:
I didn't rename the ZK dir to "managers" because that prevents the manager from starting without initialization. It seemed like too much for a minor release to require a user run some fixup script before launching 2.1.
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#issuecomment-776121664
Hey @brianloss - Do you know if client code needs to be updated with these changes? I just tried a fresh setup in Uno with these changes included and my client is hanging on a table create. I thought the old client would work but that might not be the case.
From jstack:
<pre>
at org.apache.accumulo.fate.util.UtilWaitThread.sleepUninterruptibly(UtilWaitThread.java:57)
at org.apache.accumulo.core.clientImpl.MasterClient.getConnectionWithRetry(MasterClient.java:52)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.beginFateOperation(TableOperationsImpl.java:258)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.doFateOperation(TableOperationsImpl.java:365)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.doFateOperation(TableOperationsImpl.java:355)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.doTableFateOperation(TableOperationsImpl.java:1694)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.create(TableOperationsImpl.java:246)
at org.apache.accumulo.core.clientImpl.TableOperationsImpl.create(TableOperationsImpl.java:216)
at org.apache.accumulo.testing.randomwalk.multitable.CreateTable.visit(CreateTable.java:48)
at org.apache.accumulo.testing.randomwalk.Module.visit(Module.java:237)
at org.apache.accumulo.testing.randomwalk.Framework.run(Framework.java:48)
at org.apache.accumulo.testing.randomwalk.Framework.main(Framework.java:92)
</pre>
----------------------------------------------------------------
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] brianloss commented on pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#issuecomment-776137525
> Do you know if client code needs to be updated with these changes?
@milleruntime with renaming the MasterClient class, yes, that means client code needs to be updated. I called out the incompatibility in my [PR for the release notes](https://github.com/apache/accumulo-website/pull/263/files#diff-83d7bf5ef06296a08ad334bf95e8db825a72737616f86dc20db6c69a4aa50c98R10). Before making the known breaking change, I had tried running a 2.0 client against a 2.1 server (or maybe I tested the reverse, now I don't recall for certain) and while the client came up, a scan against the metadata table hung. So I figured we already weren't compatible.
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572634175
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
Review comment:
This is fine, but will definitely need a mention in the release notes. FWIW, if 2.1 is an LTM release (and I think it should be), we won't need to keep this around, because we only need to support upgrades from the last LTM to the next.
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/namespace/create/CreateNamespace.java
##########
@@ -44,11 +44,11 @@ public long isReady(long tid, Manager environment) {
}
@Override
- public Repo<Manager> call(long tid, Manager master) throws Exception {
+ public Repo<Manager> call(long tid, Manager manager) throws Exception {
Review comment:
Most of these FaTE RepOs could just use the name "env". Some of them already do that. But this is fine, too.
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572373389
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
Review comment:
Hmm, I was checking the lack of existence of the managers dir, since the manager won't be able to start without it. Thinking through it, though, I suppose the problem would be if the recursive copy doesn't complete successfully. If we switch the check, then the concern would be if the recursive delete of /masters doesn't complete. That's probably not a problem since startup will fail. Next time around, we'll try to copy whatever's left from /masters to /managers (which should be ok since the manager never started so nothing under /managers got updated). So given that, then sure, I think it makes sense to flip the check. I'll take care of that.
----------------------------------------------------------------
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] brianloss commented on pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#issuecomment-776147456
@milleruntime, see the discussion on PR #1907 [here](https://github.com/apache/accumulo/pull/1907#discussion_r570257915).
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572369864
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
+ if (managersDirMissing) {
+ LOG.info("Renaming ZooKeeper directory {} to {}.", mastersZooDir, managersZooDir);
+ zoo.recursiveCopyPersistentOverwrite(mastersZooDir, managersZooDir);
+ zoo.recursiveDelete(mastersZooDir, ZooUtil.NodeMissingPolicy.SKIP);
Review comment:
Sure that makes sense.
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572460894
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
+ }
+ }
+
+ public static boolean renameMasterDirInZK(ServerContext ctx) {
+ final ZooReaderWriter zoo = ctx.getZooReaderWriter();
+ final String mastersZooDir = ctx.getZooKeeperRoot() + "/masters";
+ final String managersZooDir = ctx.getZooKeeperRoot() + Constants.ZMANAGERS;
+ try {
+ boolean managersDirMissing = !zoo.exists(managersZooDir);
Review comment:
The changes look good.
----------------------------------------------------------------
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 #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r571207611
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -42,11 +42,11 @@
public static final String ZNAMESPACE_NAME = "/name";
public static final String ZNAMESPACE_CONF = "/conf";
- public static final String ZMASTERS = "/masters";
- public static final String ZMASTER_LOCK = ZMASTERS + "/lock";
- public static final String ZMASTER_GOAL_STATE = ZMASTERS + "/goal_state";
- public static final String ZMASTER_REPLICATION_COORDINATOR_ADDR = ZMASTERS + "/repl_coord_addr";
- public static final String ZMASTER_TICK = ZMASTERS + "/tick";
+ public static final String ZMANAGERS = "/masters";
Review comment:
It's possible we could add some upgrade code for this. I'm not sure what would be involved, though. With upgrade code, we can start relying on more constrained upgrade paths (https://accumulo.apache.org/contributor/versioning#LTM)
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572369338
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/upgrade/RenameMasterDirInZK.java
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.manager.upgrade;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.conf.SiteConfiguration;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.fate.zookeeper.ZooUtil;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A utility to handle the renaming of "/masters" to "/managers" in Zookeeper when upgrading from a
+ * 2.0 (or earlier) to 2.1 instance. This utility is invoked automatically by
+ * {@link org.apache.accumulo.manager.state.SetGoalState} (which normally runs first as a part of
+ * accumulo startup scripts). However, if a user is not using the standard scripts or wishes to
+ * perform the upgrade as a separate process, this utility can be invoked with:
+ *
+ * <pre>
+ * {@code
+ * bin/accumulo org.apache.accumulo.manager.upgrade.RenameMasterDirInZK
+ * }
+ * </pre>
+ */
+public class RenameMasterDirInZK {
+ private static final Logger LOG = LoggerFactory.getLogger(RenameMasterDirInZK.class);
+
+ public static void main(String[] args) {
+ var ctx = new ServerContext(SiteConfiguration.auto());
+ if (!renameMasterDirInZK(ctx)) {
+ LOG.info("Managers directory already exists in ZooKeeper. Not attempting rename.");
Review comment:
No, it will only show up if you run this utility directly. In that case, I figured you'd want a message so the tool tells you it did nothing. When this is invoked from SetGoalState the renameMasterDirInZK is called, the main method, so this log would not happen.
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r571213979
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -42,11 +42,11 @@
public static final String ZNAMESPACE_NAME = "/name";
public static final String ZNAMESPACE_CONF = "/conf";
- public static final String ZMASTERS = "/masters";
- public static final String ZMASTER_LOCK = ZMASTERS + "/lock";
- public static final String ZMASTER_GOAL_STATE = ZMASTERS + "/goal_state";
- public static final String ZMASTER_REPLICATION_COORDINATOR_ADDR = ZMASTERS + "/repl_coord_addr";
- public static final String ZMASTER_TICK = ZMASTERS + "/tick";
+ public static final String ZMANAGERS = "/masters";
Review comment:
From a ZK upgrade, I believe it's straightforward--do a recursive copy of /masters to /managers and then delete /masters. However, the existing upgrader runs inside the manager which won't start because the accumulo-service script invokes [SetGoalState](https://github.com/apache/accumulo/blob/main/assemble/bin/accumulo-service#L72) first and that hangs. I suppose I could add a check in SetGoalState to do the upgrade if /masters exists and /managers doesn't. Not sure if that makes sense or not.
----------------------------------------------------------------
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] brianloss commented on a change in pull request #1912: Rename master comment/constant/variable references.
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #1912:
URL: https://github.com/apache/accumulo/pull/1912#discussion_r572197097
##########
File path: core/src/main/java/org/apache/accumulo/core/Constants.java
##########
@@ -42,11 +42,11 @@
public static final String ZNAMESPACE_NAME = "/name";
public static final String ZNAMESPACE_CONF = "/conf";
- public static final String ZMASTERS = "/masters";
- public static final String ZMASTER_LOCK = ZMASTERS + "/lock";
- public static final String ZMASTER_GOAL_STATE = ZMASTERS + "/goal_state";
- public static final String ZMASTER_REPLICATION_COORDINATOR_ADDR = ZMASTERS + "/repl_coord_addr";
- public static final String ZMASTER_TICK = ZMASTERS + "/tick";
+ public static final String ZMANAGERS = "/masters";
Review comment:
That sounds good. I pushed a commit that renames the constant and adds a conversion util that is invoked from SetGoalState (but can also be run separately).
----------------------------------------------------------------
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