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 2022/03/02 19:33:03 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2541: Refactor FateCommand to be testable
milleruntime opened a new pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541
* Also create FateCommand test
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059436437
That's fine. I will work on those conflicts and the other requested changes
on that PR.
On Fri, Mar 4, 2022 at 1:53 PM Mike Miller ***@***.***> wrote:
> This PR is a lot simpler than #2215
> <https://github.com/apache/accumulo/pull/2215> and just touches server
> code. I think this one can be merged but @Manno15
> <https://github.com/Manno15> this will definitely cause conflicts with
> your changes in #2215 <https://github.com/apache/accumulo/pull/2215> . It
> will be nice to have the Unit test to do more testing with FateCommand.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/2541#issuecomment-1059429649>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHASSV3FTOBCQGYGZ6RXRPLU6JL2RANCNFSM5PYMYBXA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059845842
> Just to note, most of the suggestions for `FateCommand` will be altered quite a bit in #2215.
Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that? @milleruntime , thoughts?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059969929
> Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that?
The other changes in this PR, specifically the test, are still valuable to have. I am currently working on incorporating changes from this PR into #2215. This PR can be merged in first and then it should be a smooth transition to #2215. Though, the test might have to become an IT.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime edited a comment on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime edited a comment on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059274858
> That PR should be updated with main now. It has been ready for review for a little while now but has fallen to the wayside due to priority on other things. I am not quite sure where it stands as a priority right now.
OK thanks. It looks like you added new APIs. I will take a look at the PR.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059969929
> Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that?
The other changes in this PR, specifically the test, are still valuable to have. I am currently working on incorporating changes from this PR into #2215. This PR can be merged in first and then it should be a smooth transition to #2215. Converting the test to work with the API additions in #2215 has taken longer than expected.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059969929
> Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that?
The other changes in this PR, specifically the test, are still valuable to have. I am currently working on incorporating changes from this PR into #2215. This PR can be merged in first and then it should be a smooth transition to #2215.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059825271
Just to note, most of the suggestions for `FateCommand` will be altered quite a bit in #2215.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#discussion_r820120304
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.shell.commands;
+
+import static org.apache.accumulo.fate.zookeeper.ServiceLock.ServiceLockPath;
+import static org.easymock.EasyMock.replay;
Review comment:
You have a mix of static imported EasyMock stuff and non-static imports. I think the code would be cleaner if you statically imported all the `EasyMock.*` calls.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -130,107 +132,131 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
AdminUtil<FateCommand> admin = new AdminUtil<>(false);
- String path = context.getZooKeeperRoot() + Constants.ZFATE;
+ String fatePath = context.getZooKeeperRoot() + Constants.ZFATE;
var managerLockPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZMANAGER_LOCK);
+ var tableLocksPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS);
ZooReaderWriter zk =
getZooReaderWriter(context, siteConfig, cl.getOptionValue(secretOption.getOpt()));
- ZooStore<FateCommand> zs = new ZooStore<>(path, zk);
+ ZooStore<FateCommand> zs = new ZooStore<>(fatePath, zk);
if ("fail".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
- for (int i = 1; i < args.length; i++) {
- if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
- System.out.printf("Could not fail transaction: %s%n", args[i]);
- failedCommand = true;
- }
- }
+ validateArgs(args);
+ failedCommand = failTx(admin, zs, zk, managerLockPath, args);
} else if ("delete".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
+ validateArgs(args);
+ failedCommand = deleteTx(admin, zs, zk, managerLockPath, args);
+ } else if ("list".equals(cmd) || "print".equals(cmd)) {
+ printTx(shellState, admin, zs, zk, tableLocksPath, args, cl,
+ cl.hasOption(statusOption.getOpt()));
+ } else if ("dump".equals(cmd)) {
+ String output = dumpTx(zs, args);
+ System.out.println(output);
+ } else {
+ throw new ParseException("Invalid command option");
+ }
+
+ return failedCommand ? 1 : 0;
+ }
+
+ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
+ List<Long> txids;
+ if (args.length == 1) {
+ txids = zs.list();
+ } else {
+ txids = new ArrayList<>();
for (int i = 1; i < args.length; i++) {
- if (admin.prepDelete(zs, zk, managerLockPath, args[i])) {
- admin.deleteLocks(zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, args[i]);
- } else {
- System.out.printf("Could not delete transaction: %s%n", args[i]);
- failedCommand = true;
- }
+ txids.add(parseTxid(args[i]));
}
- } else if ("list".equals(cmd) || "print".equals(cmd)) {
- // Parse transaction ID filters for print display
- Set<Long> filterTxid = null;
- if (args.length >= 2) {
- filterTxid = new HashSet<>(args.length);
- for (int i = 1; i < args.length; i++) {
- try {
- Long val = parseTxid(args[i]);
- filterTxid.add(val);
- } catch (NumberFormatException nfe) {
- // Failed to parse, will exit instead of displaying everything since the intention was
- // to potentially filter some data
- System.out.printf("Invalid transaction ID format: %s%n", args[i]);
- return 1;
- }
+ }
+
+ Gson gson = new GsonBuilder()
+ .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();
+
+ List<FateStack> txStacks = new ArrayList<>();
+ for (Long txid : txids) {
+ List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
+ txStacks.add(new FateStack(txid, repoStack));
+ }
+
+ return gson.toJson(txStacks);
+ }
+
+ private void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLock.ServiceLockPath tableLocksPath, String[] args, CommandLine cl,
+ boolean printStatus) throws InterruptedException, KeeperException, IOException {
+ // Parse transaction ID filters for print display
+ Set<Long> filterTxid = null;
+ if (args.length >= 2) {
+ filterTxid = new HashSet<>(args.length);
+ for (int i = 1; i < args.length; i++) {
+ try {
+ Long val = parseTxid(args[i]);
+ filterTxid.add(val);
+ } catch (NumberFormatException nfe) {
+ // Failed to parse, will exit instead of displaying everything since the intention was
+ // to potentially filter some data
+ throw new RuntimeException("Invalid transaction ID format: " + args[i], nfe);
}
}
+ }
- // Parse TStatus filters for print display
- EnumSet<TStatus> filterStatus = null;
- if (cl.hasOption(statusOption.getOpt())) {
- filterStatus = EnumSet.noneOf(TStatus.class);
- String[] tstat = cl.getOptionValues(statusOption.getOpt());
- for (String element : tstat) {
- try {
- filterStatus.add(TStatus.valueOf(element));
- } catch (IllegalArgumentException iae) {
- System.out.printf("Invalid transaction status name: %s%n", element);
- return 1;
- }
+ // Parse TStatus filters for print display
+ EnumSet<TStatus> filterStatus = null;
+ if (printStatus) {
+ filterStatus = EnumSet.noneOf(TStatus.class);
+ String[] tstat = cl.getOptionValues(statusOption.getOpt());
+ for (String element : tstat) {
+ try {
+ filterStatus.add(TStatus.valueOf(element));
+ } catch (IllegalArgumentException iae) {
+ throw new RuntimeException("Invalid transaction status name: " + element, iae);
}
}
+ }
- StringBuilder buf = new StringBuilder(8096);
- Formatter fmt = new Formatter(buf);
- admin.print(zs, zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, fmt, filterTxid,
- filterStatus);
- shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
- !cl.hasOption(disablePaginationOpt.getOpt()));
- } else if ("dump".equals(cmd)) {
- List<Long> txids;
+ StringBuilder buf = new StringBuilder(8096);
+ Formatter fmt = new Formatter(buf);
+ admin.print(zs, zk, tableLocksPath, fmt, filterTxid, filterStatus);
+ shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
+ !cl.hasOption(disablePaginationOpt.getOpt()));
+ }
- if (args.length == 1) {
- txids = zs.list();
+ private boolean deleteTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
+ throws InterruptedException, KeeperException {
+ boolean success = true;
+ for (int i = 1; i < args.length; i++) {
+ if (admin.prepDelete(zs, zk, zLockManagerPath, args[i])) {
+ admin.deleteLocks(zk, zLockManagerPath, args[i]);
} else {
- txids = new ArrayList<>();
- for (int i = 1; i < args.length; i++) {
- txids.add(parseTxid(args[i]));
- }
+ System.out.printf("Could not delete transaction: %s%n", args[i]);
+ return !success;
}
+ }
+ return success;
+ }
- Gson gson =
- new GsonBuilder().registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
- .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
- .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting()
- .create();
-
- List<FateStack> txStacks = new ArrayList<>();
+ private void validateArgs(String[] args) throws ParseException {
+ if (args.length <= 1) {
+ throw new ParseException("Must provide transaction ID");
+ }
+ }
- for (Long txid : txids) {
- List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
- txStacks.add(new FateStack(txid, repoStack));
+ public boolean failTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs, ZooReaderWriter zk,
+ ServiceLockPath managerLockPath, String[] args) {
+ boolean success = true;
+ for (int i = 1; i < args.length; i++) {
+ if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
+ System.out.printf("Could not fail transaction: %s%n", args[i]);
+ return !success;
}
-
- System.out.println(gson.toJson(txStacks));
- } else {
- throw new ParseException("Invalid command option");
}
-
- return failedCommand ? 1 : 0;
+ return success;
}
- protected synchronized ZooReaderWriter getZooReaderWriter(ClientContext context,
+ synchronized ZooReaderWriter getZooReaderWriter(ClientContext context,
SiteConfiguration siteConfig, String secret) {
if (secret == null) {
Review comment:
I'm not sure why this should be synchronized. There's no state being preserved or accessed outside this method from what I can tell.
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -130,107 +132,131 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
AdminUtil<FateCommand> admin = new AdminUtil<>(false);
- String path = context.getZooKeeperRoot() + Constants.ZFATE;
+ String fatePath = context.getZooKeeperRoot() + Constants.ZFATE;
var managerLockPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZMANAGER_LOCK);
+ var tableLocksPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS);
ZooReaderWriter zk =
getZooReaderWriter(context, siteConfig, cl.getOptionValue(secretOption.getOpt()));
- ZooStore<FateCommand> zs = new ZooStore<>(path, zk);
+ ZooStore<FateCommand> zs = new ZooStore<>(fatePath, zk);
if ("fail".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
- for (int i = 1; i < args.length; i++) {
- if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
- System.out.printf("Could not fail transaction: %s%n", args[i]);
- failedCommand = true;
- }
- }
+ validateArgs(args);
+ failedCommand = failTx(admin, zs, zk, managerLockPath, args);
} else if ("delete".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
+ validateArgs(args);
+ failedCommand = deleteTx(admin, zs, zk, managerLockPath, args);
+ } else if ("list".equals(cmd) || "print".equals(cmd)) {
+ printTx(shellState, admin, zs, zk, tableLocksPath, args, cl,
+ cl.hasOption(statusOption.getOpt()));
+ } else if ("dump".equals(cmd)) {
+ String output = dumpTx(zs, args);
+ System.out.println(output);
+ } else {
+ throw new ParseException("Invalid command option");
+ }
+
+ return failedCommand ? 1 : 0;
+ }
+
+ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
+ List<Long> txids;
+ if (args.length == 1) {
+ txids = zs.list();
+ } else {
+ txids = new ArrayList<>();
for (int i = 1; i < args.length; i++) {
- if (admin.prepDelete(zs, zk, managerLockPath, args[i])) {
- admin.deleteLocks(zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, args[i]);
- } else {
- System.out.printf("Could not delete transaction: %s%n", args[i]);
- failedCommand = true;
- }
+ txids.add(parseTxid(args[i]));
}
- } else if ("list".equals(cmd) || "print".equals(cmd)) {
- // Parse transaction ID filters for print display
- Set<Long> filterTxid = null;
- if (args.length >= 2) {
- filterTxid = new HashSet<>(args.length);
- for (int i = 1; i < args.length; i++) {
- try {
- Long val = parseTxid(args[i]);
- filterTxid.add(val);
- } catch (NumberFormatException nfe) {
- // Failed to parse, will exit instead of displaying everything since the intention was
- // to potentially filter some data
- System.out.printf("Invalid transaction ID format: %s%n", args[i]);
- return 1;
- }
+ }
+
+ Gson gson = new GsonBuilder()
+ .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();
+
+ List<FateStack> txStacks = new ArrayList<>();
+ for (Long txid : txids) {
+ List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
+ txStacks.add(new FateStack(txid, repoStack));
+ }
+
+ return gson.toJson(txStacks);
+ }
+
+ private void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLock.ServiceLockPath tableLocksPath, String[] args, CommandLine cl,
+ boolean printStatus) throws InterruptedException, KeeperException, IOException {
+ // Parse transaction ID filters for print display
+ Set<Long> filterTxid = null;
+ if (args.length >= 2) {
+ filterTxid = new HashSet<>(args.length);
+ for (int i = 1; i < args.length; i++) {
+ try {
+ Long val = parseTxid(args[i]);
+ filterTxid.add(val);
+ } catch (NumberFormatException nfe) {
+ // Failed to parse, will exit instead of displaying everything since the intention was
+ // to potentially filter some data
+ throw new RuntimeException("Invalid transaction ID format: " + args[i], nfe);
}
}
+ }
- // Parse TStatus filters for print display
- EnumSet<TStatus> filterStatus = null;
- if (cl.hasOption(statusOption.getOpt())) {
- filterStatus = EnumSet.noneOf(TStatus.class);
- String[] tstat = cl.getOptionValues(statusOption.getOpt());
- for (String element : tstat) {
- try {
- filterStatus.add(TStatus.valueOf(element));
- } catch (IllegalArgumentException iae) {
- System.out.printf("Invalid transaction status name: %s%n", element);
- return 1;
- }
+ // Parse TStatus filters for print display
+ EnumSet<TStatus> filterStatus = null;
+ if (printStatus) {
+ filterStatus = EnumSet.noneOf(TStatus.class);
+ String[] tstat = cl.getOptionValues(statusOption.getOpt());
+ for (String element : tstat) {
+ try {
+ filterStatus.add(TStatus.valueOf(element));
+ } catch (IllegalArgumentException iae) {
+ throw new RuntimeException("Invalid transaction status name: " + element, iae);
Review comment:
This, and in other places, you're converting a specific RTE to a more generic RTE. I think there's more value in keeping specific RTEs.
Additionally, the intent of the previous code looks like it was trying to communicate with the user by translating a well-understood exception into a message that would be helpful to the user. This new code rethrows the exception, and depending on the local log4j settings, the user may not even see it in the console. If they do see it, they'll see a very verbose stack trace that's probably unnecessary for communicating the error in the malformed transaction status name.
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.shell.commands;
+
+import static org.apache.accumulo.fate.zookeeper.ServiceLock.ServiceLockPath;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+
+import org.apache.accumulo.fate.AdminUtil;
+import org.apache.accumulo.fate.ReadOnlyRepo;
+import org.apache.accumulo.fate.ReadOnlyTStore;
+import org.apache.accumulo.fate.ZooStore;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.easymock.EasyMock;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class FateCommandTest {
+ private static ZooReaderWriter zk;
+ private static ServiceLockPath managerLockPath;
+
+ @BeforeClass
+ public static void setup() {
+ zk = EasyMock.createMock(ZooReaderWriter.class);
+ managerLockPath = EasyMock.createMock(ServiceLockPath.class);
+ }
+
+ @Test
+ public void testFailTx() throws Exception {
+ ZooStore<FateCommand> zs = EasyMock.createMock(ZooStore.class);
+ String tidStr = "12345";
+ long tid = Long.parseLong(tidStr, 16);
+ EasyMock.expect(zs.getStatus(tid)).andReturn(ReadOnlyTStore.TStatus.NEW).anyTimes();
+ zs.reserve(tid);
+ EasyMock.expectLastCall().once();
+ zs.setStatus(tid, ReadOnlyTStore.TStatus.FAILED_IN_PROGRESS);
+ EasyMock.expectLastCall().once();
+ zs.unreserve(tid, 0);
+ EasyMock.expectLastCall().once();
+
+ TestHelper helper = new TestHelper(true);
+
+ replay(zs);
Review comment:
I don't see a `verify` call anywhere. It's good practice: `create mocks -> enter replay mode -> verify mock objects`
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -130,107 +132,131 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
AdminUtil<FateCommand> admin = new AdminUtil<>(false);
- String path = context.getZooKeeperRoot() + Constants.ZFATE;
+ String fatePath = context.getZooKeeperRoot() + Constants.ZFATE;
var managerLockPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZMANAGER_LOCK);
+ var tableLocksPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS);
ZooReaderWriter zk =
getZooReaderWriter(context, siteConfig, cl.getOptionValue(secretOption.getOpt()));
- ZooStore<FateCommand> zs = new ZooStore<>(path, zk);
+ ZooStore<FateCommand> zs = new ZooStore<>(fatePath, zk);
if ("fail".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
- for (int i = 1; i < args.length; i++) {
- if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
- System.out.printf("Could not fail transaction: %s%n", args[i]);
- failedCommand = true;
- }
- }
+ validateArgs(args);
+ failedCommand = failTx(admin, zs, zk, managerLockPath, args);
} else if ("delete".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
+ validateArgs(args);
+ failedCommand = deleteTx(admin, zs, zk, managerLockPath, args);
+ } else if ("list".equals(cmd) || "print".equals(cmd)) {
+ printTx(shellState, admin, zs, zk, tableLocksPath, args, cl,
+ cl.hasOption(statusOption.getOpt()));
+ } else if ("dump".equals(cmd)) {
+ String output = dumpTx(zs, args);
+ System.out.println(output);
+ } else {
+ throw new ParseException("Invalid command option");
+ }
+
+ return failedCommand ? 1 : 0;
+ }
+
+ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
+ List<Long> txids;
+ if (args.length == 1) {
+ txids = zs.list();
+ } else {
+ txids = new ArrayList<>();
for (int i = 1; i < args.length; i++) {
- if (admin.prepDelete(zs, zk, managerLockPath, args[i])) {
- admin.deleteLocks(zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, args[i]);
- } else {
- System.out.printf("Could not delete transaction: %s%n", args[i]);
- failedCommand = true;
- }
+ txids.add(parseTxid(args[i]));
}
- } else if ("list".equals(cmd) || "print".equals(cmd)) {
- // Parse transaction ID filters for print display
- Set<Long> filterTxid = null;
- if (args.length >= 2) {
- filterTxid = new HashSet<>(args.length);
- for (int i = 1; i < args.length; i++) {
- try {
- Long val = parseTxid(args[i]);
- filterTxid.add(val);
- } catch (NumberFormatException nfe) {
- // Failed to parse, will exit instead of displaying everything since the intention was
- // to potentially filter some data
- System.out.printf("Invalid transaction ID format: %s%n", args[i]);
- return 1;
- }
+ }
+
+ Gson gson = new GsonBuilder()
+ .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();
+
+ List<FateStack> txStacks = new ArrayList<>();
+ for (Long txid : txids) {
+ List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
+ txStacks.add(new FateStack(txid, repoStack));
+ }
+
+ return gson.toJson(txStacks);
+ }
+
+ private void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLock.ServiceLockPath tableLocksPath, String[] args, CommandLine cl,
+ boolean printStatus) throws InterruptedException, KeeperException, IOException {
+ // Parse transaction ID filters for print display
+ Set<Long> filterTxid = null;
+ if (args.length >= 2) {
+ filterTxid = new HashSet<>(args.length);
+ for (int i = 1; i < args.length; i++) {
+ try {
+ Long val = parseTxid(args[i]);
+ filterTxid.add(val);
+ } catch (NumberFormatException nfe) {
+ // Failed to parse, will exit instead of displaying everything since the intention was
+ // to potentially filter some data
+ throw new RuntimeException("Invalid transaction ID format: " + args[i], nfe);
}
}
+ }
- // Parse TStatus filters for print display
- EnumSet<TStatus> filterStatus = null;
- if (cl.hasOption(statusOption.getOpt())) {
- filterStatus = EnumSet.noneOf(TStatus.class);
- String[] tstat = cl.getOptionValues(statusOption.getOpt());
- for (String element : tstat) {
- try {
- filterStatus.add(TStatus.valueOf(element));
- } catch (IllegalArgumentException iae) {
- System.out.printf("Invalid transaction status name: %s%n", element);
- return 1;
- }
+ // Parse TStatus filters for print display
+ EnumSet<TStatus> filterStatus = null;
+ if (printStatus) {
+ filterStatus = EnumSet.noneOf(TStatus.class);
+ String[] tstat = cl.getOptionValues(statusOption.getOpt());
+ for (String element : tstat) {
+ try {
+ filterStatus.add(TStatus.valueOf(element));
+ } catch (IllegalArgumentException iae) {
+ throw new RuntimeException("Invalid transaction status name: " + element, iae);
}
}
+ }
- StringBuilder buf = new StringBuilder(8096);
- Formatter fmt = new Formatter(buf);
- admin.print(zs, zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, fmt, filterTxid,
- filterStatus);
- shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
- !cl.hasOption(disablePaginationOpt.getOpt()));
- } else if ("dump".equals(cmd)) {
- List<Long> txids;
+ StringBuilder buf = new StringBuilder(8096);
+ Formatter fmt = new Formatter(buf);
+ admin.print(zs, zk, tableLocksPath, fmt, filterTxid, filterStatus);
+ shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
+ !cl.hasOption(disablePaginationOpt.getOpt()));
+ }
- if (args.length == 1) {
- txids = zs.list();
+ private boolean deleteTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
+ throws InterruptedException, KeeperException {
+ boolean success = true;
Review comment:
This looks like it's being used as a constant to represent SUCCESS, and not the actual variable value of whether or not it succeeded. As such, it should probably be final, and maybe a static final constant, all upper-case, so it doesn't look like a variable containing a result.
So, something like:
```java
final SUCCESS = true;
// ...
return !SUCCESS;
// ...
return SUCCESS;
```
However, as a developer looking at this code, I'm not really sure what this is doing unless I look at the assignments to see what value is contained in SUCCESS. So, it'd probably just be cleaner to directly return `true` and `false`, rather than negating a variable or a constant whose value is unknown to the developer until they go look.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1058446761
I haven't fully looked into it to see the similarities, but I do still have an open PR (https://github.com/apache/accumulo/pull/2215) that does make a decent amount of changes to `FateCommand`. I will need to go back to update it with main though.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059969929
> Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that?
The other changes in this PR, specifically the test, are still valuable to have. I am currently working on incorporating changes from this PR into #2215. This PR can be merged in first and then it should be a smooth transition to #2215.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059177742
> I haven't fully looked into it to see the similarities, but I do still have an open PR (#2215) that does make a decent amount of changes to `FateCommand`. I will need to go back to update it with main though.
Ah OK. I hadn't seen that PR. FYI this one is ready to go. Let me know if you get that one updated.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059429649
This PR is a lot simpler than #2215 and just touches server code. I think this one can be merged but @Manno15 this will definitely cause conflicts with your changes in #2215 . It will be nice to have the Unit test to do more testing with FateCommand.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059186716
That PR should be updated with main now. It has been ready for review for a
little while now but has fallen to the wayside due to priority on other
things. I am not quite sure where it stands as a priority right now.
On Fri, Mar 4, 2022 at 8:49 AM Mike Miller ***@***.***> wrote:
> I haven't fully looked into it to see the similarities, but I do still
> have an open PR (#2215 <https://github.com/apache/accumulo/pull/2215>)
> that does make a decent amount of changes to FateCommand. I will need to
> go back to update it with main though.
>
> Ah OK. I hadn't seen that PR. FYI this one is ready to go. Let me know if
> you get that one updated.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/accumulo/pull/2541#issuecomment-1059177742>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHASSV4RSPVAA3WHX65JZHLU6IIG7ANCNFSM5PYMYBXA>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#discussion_r821652868
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.shell.commands;
+
+import static org.apache.accumulo.fate.zookeeper.ServiceLock.ServiceLockPath;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.util.List;
+
+import org.apache.accumulo.fate.AdminUtil;
+import org.apache.accumulo.fate.ReadOnlyRepo;
+import org.apache.accumulo.fate.ReadOnlyTStore;
+import org.apache.accumulo.fate.ZooStore;
+import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
+import org.easymock.EasyMock;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class FateCommandTest {
+ private static ZooReaderWriter zk;
+ private static ServiceLockPath managerLockPath;
+
+ @BeforeClass
+ public static void setup() {
+ zk = EasyMock.createMock(ZooReaderWriter.class);
+ managerLockPath = EasyMock.createMock(ServiceLockPath.class);
+ }
+
+ @Test
+ public void testFailTx() throws Exception {
+ ZooStore<FateCommand> zs = EasyMock.createMock(ZooStore.class);
+ String tidStr = "12345";
+ long tid = Long.parseLong(tidStr, 16);
+ EasyMock.expect(zs.getStatus(tid)).andReturn(ReadOnlyTStore.TStatus.NEW).anyTimes();
+ zs.reserve(tid);
+ EasyMock.expectLastCall().once();
+ zs.setStatus(tid, ReadOnlyTStore.TStatus.FAILED_IN_PROGRESS);
+ EasyMock.expectLastCall().once();
+ zs.unreserve(tid, 0);
+ EasyMock.expectLastCall().once();
+
+ TestHelper helper = new TestHelper(true);
+
+ replay(zs);
Review comment:
Good catch. Fixed in af358ef1bc0153873959ba821b67d0877206ead8
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#discussion_r821697983
##########
File path: shell/src/test/java/org/apache/accumulo/shell/commands/FateCommandTest.java
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.shell.commands;
+
+import static org.apache.accumulo.fate.zookeeper.ServiceLock.ServiceLockPath;
+import static org.easymock.EasyMock.replay;
Review comment:
Fixed in 1a68628b0a299ff0df139056060a52a9fd5f72a2
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#discussion_r821690750
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -130,107 +132,131 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
AdminUtil<FateCommand> admin = new AdminUtil<>(false);
- String path = context.getZooKeeperRoot() + Constants.ZFATE;
+ String fatePath = context.getZooKeeperRoot() + Constants.ZFATE;
var managerLockPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZMANAGER_LOCK);
+ var tableLocksPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS);
ZooReaderWriter zk =
getZooReaderWriter(context, siteConfig, cl.getOptionValue(secretOption.getOpt()));
- ZooStore<FateCommand> zs = new ZooStore<>(path, zk);
+ ZooStore<FateCommand> zs = new ZooStore<>(fatePath, zk);
if ("fail".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
- for (int i = 1; i < args.length; i++) {
- if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
- System.out.printf("Could not fail transaction: %s%n", args[i]);
- failedCommand = true;
- }
- }
+ validateArgs(args);
+ failedCommand = failTx(admin, zs, zk, managerLockPath, args);
} else if ("delete".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
+ validateArgs(args);
+ failedCommand = deleteTx(admin, zs, zk, managerLockPath, args);
+ } else if ("list".equals(cmd) || "print".equals(cmd)) {
+ printTx(shellState, admin, zs, zk, tableLocksPath, args, cl,
+ cl.hasOption(statusOption.getOpt()));
+ } else if ("dump".equals(cmd)) {
+ String output = dumpTx(zs, args);
+ System.out.println(output);
+ } else {
+ throw new ParseException("Invalid command option");
+ }
+
+ return failedCommand ? 1 : 0;
+ }
+
+ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
+ List<Long> txids;
+ if (args.length == 1) {
+ txids = zs.list();
+ } else {
+ txids = new ArrayList<>();
for (int i = 1; i < args.length; i++) {
- if (admin.prepDelete(zs, zk, managerLockPath, args[i])) {
- admin.deleteLocks(zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, args[i]);
- } else {
- System.out.printf("Could not delete transaction: %s%n", args[i]);
- failedCommand = true;
- }
+ txids.add(parseTxid(args[i]));
}
- } else if ("list".equals(cmd) || "print".equals(cmd)) {
- // Parse transaction ID filters for print display
- Set<Long> filterTxid = null;
- if (args.length >= 2) {
- filterTxid = new HashSet<>(args.length);
- for (int i = 1; i < args.length; i++) {
- try {
- Long val = parseTxid(args[i]);
- filterTxid.add(val);
- } catch (NumberFormatException nfe) {
- // Failed to parse, will exit instead of displaying everything since the intention was
- // to potentially filter some data
- System.out.printf("Invalid transaction ID format: %s%n", args[i]);
- return 1;
- }
+ }
+
+ Gson gson = new GsonBuilder()
+ .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();
+
+ List<FateStack> txStacks = new ArrayList<>();
+ for (Long txid : txids) {
+ List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
+ txStacks.add(new FateStack(txid, repoStack));
+ }
+
+ return gson.toJson(txStacks);
+ }
+
+ private void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLock.ServiceLockPath tableLocksPath, String[] args, CommandLine cl,
+ boolean printStatus) throws InterruptedException, KeeperException, IOException {
+ // Parse transaction ID filters for print display
+ Set<Long> filterTxid = null;
+ if (args.length >= 2) {
+ filterTxid = new HashSet<>(args.length);
+ for (int i = 1; i < args.length; i++) {
+ try {
+ Long val = parseTxid(args[i]);
+ filterTxid.add(val);
+ } catch (NumberFormatException nfe) {
+ // Failed to parse, will exit instead of displaying everything since the intention was
+ // to potentially filter some data
+ throw new RuntimeException("Invalid transaction ID format: " + args[i], nfe);
}
}
+ }
- // Parse TStatus filters for print display
- EnumSet<TStatus> filterStatus = null;
- if (cl.hasOption(statusOption.getOpt())) {
- filterStatus = EnumSet.noneOf(TStatus.class);
- String[] tstat = cl.getOptionValues(statusOption.getOpt());
- for (String element : tstat) {
- try {
- filterStatus.add(TStatus.valueOf(element));
- } catch (IllegalArgumentException iae) {
- System.out.printf("Invalid transaction status name: %s%n", element);
- return 1;
- }
+ // Parse TStatus filters for print display
+ EnumSet<TStatus> filterStatus = null;
+ if (printStatus) {
+ filterStatus = EnumSet.noneOf(TStatus.class);
+ String[] tstat = cl.getOptionValues(statusOption.getOpt());
+ for (String element : tstat) {
+ try {
+ filterStatus.add(TStatus.valueOf(element));
+ } catch (IllegalArgumentException iae) {
+ throw new RuntimeException("Invalid transaction status name: " + element, iae);
Review comment:
Fixed in c33b34aef77d4d1dfe9a31eca650983f3f714def
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] Manno15 edited a comment on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
Manno15 edited a comment on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059969929
> Should this be collapsed into that, then? Do we still need this if it's going to be superseded by that?
The other changes in this PR, specifically the test, are still valuable to have. I am currently working on incorporating changes from this PR into #2215. This PR can be merged in first and then it should be a smooth transition to #2215. Converting the test to work with the API additions in #2215 has been more complicated than expected.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#issuecomment-1059274858
> That PR should be updated with main now. It has been ready for review for a little while now but has fallen to the wayside due to priority on other things. I am not quite sure where it stands as a priority right now.
> […](#)
> On Fri, Mar 4, 2022 at 8:49 AM Mike Miller ***@***.***> wrote: I haven't fully looked into it to see the similarities, but I do still have an open PR (#2215 <#2215>) that does make a decent amount of changes to FateCommand. I will need to go back to update it with main though. Ah OK. I hadn't seen that PR. FYI this one is ready to go. Let me know if you get that one updated. — Reply to this email directly, view it on GitHub <[#2541 (comment)](https://github.com/apache/accumulo/pull/2541#issuecomment-1059177742)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHASSV4RSPVAA3WHX65JZHLU6IIG7ANCNFSM5PYMYBXA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented
.Message ID: ***@***.***>
OK thanks. It looks like you added new APIs. I will take a look at the PR.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2541: Refactor FateCommand to be testable
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2541:
URL: https://github.com/apache/accumulo/pull/2541#discussion_r821691147
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/FateCommand.java
##########
@@ -130,107 +132,131 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
AdminUtil<FateCommand> admin = new AdminUtil<>(false);
- String path = context.getZooKeeperRoot() + Constants.ZFATE;
+ String fatePath = context.getZooKeeperRoot() + Constants.ZFATE;
var managerLockPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZMANAGER_LOCK);
+ var tableLocksPath = ServiceLock.path(context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS);
ZooReaderWriter zk =
getZooReaderWriter(context, siteConfig, cl.getOptionValue(secretOption.getOpt()));
- ZooStore<FateCommand> zs = new ZooStore<>(path, zk);
+ ZooStore<FateCommand> zs = new ZooStore<>(fatePath, zk);
if ("fail".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
- for (int i = 1; i < args.length; i++) {
- if (!admin.prepFail(zs, zk, managerLockPath, args[i])) {
- System.out.printf("Could not fail transaction: %s%n", args[i]);
- failedCommand = true;
- }
- }
+ validateArgs(args);
+ failedCommand = failTx(admin, zs, zk, managerLockPath, args);
} else if ("delete".equals(cmd)) {
- if (args.length <= 1) {
- throw new ParseException("Must provide transaction ID");
- }
+ validateArgs(args);
+ failedCommand = deleteTx(admin, zs, zk, managerLockPath, args);
+ } else if ("list".equals(cmd) || "print".equals(cmd)) {
+ printTx(shellState, admin, zs, zk, tableLocksPath, args, cl,
+ cl.hasOption(statusOption.getOpt()));
+ } else if ("dump".equals(cmd)) {
+ String output = dumpTx(zs, args);
+ System.out.println(output);
+ } else {
+ throw new ParseException("Invalid command option");
+ }
+
+ return failedCommand ? 1 : 0;
+ }
+
+ String dumpTx(ZooStore<FateCommand> zs, String[] args) {
+ List<Long> txids;
+ if (args.length == 1) {
+ txids = zs.list();
+ } else {
+ txids = new ArrayList<>();
for (int i = 1; i < args.length; i++) {
- if (admin.prepDelete(zs, zk, managerLockPath, args[i])) {
- admin.deleteLocks(zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, args[i]);
- } else {
- System.out.printf("Could not delete transaction: %s%n", args[i]);
- failedCommand = true;
- }
+ txids.add(parseTxid(args[i]));
}
- } else if ("list".equals(cmd) || "print".equals(cmd)) {
- // Parse transaction ID filters for print display
- Set<Long> filterTxid = null;
- if (args.length >= 2) {
- filterTxid = new HashSet<>(args.length);
- for (int i = 1; i < args.length; i++) {
- try {
- Long val = parseTxid(args[i]);
- filterTxid.add(val);
- } catch (NumberFormatException nfe) {
- // Failed to parse, will exit instead of displaying everything since the intention was
- // to potentially filter some data
- System.out.printf("Invalid transaction ID format: %s%n", args[i]);
- return 1;
- }
+ }
+
+ Gson gson = new GsonBuilder()
+ .registerTypeAdapter(ReadOnlyRepo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(Repo.class, new InterfaceSerializer<>())
+ .registerTypeAdapter(byte[].class, new ByteArraySerializer()).setPrettyPrinting().create();
+
+ List<FateStack> txStacks = new ArrayList<>();
+ for (Long txid : txids) {
+ List<ReadOnlyRepo<FateCommand>> repoStack = zs.getStack(txid);
+ txStacks.add(new FateStack(txid, repoStack));
+ }
+
+ return gson.toJson(txStacks);
+ }
+
+ private void printTx(Shell shellState, AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLock.ServiceLockPath tableLocksPath, String[] args, CommandLine cl,
+ boolean printStatus) throws InterruptedException, KeeperException, IOException {
+ // Parse transaction ID filters for print display
+ Set<Long> filterTxid = null;
+ if (args.length >= 2) {
+ filterTxid = new HashSet<>(args.length);
+ for (int i = 1; i < args.length; i++) {
+ try {
+ Long val = parseTxid(args[i]);
+ filterTxid.add(val);
+ } catch (NumberFormatException nfe) {
+ // Failed to parse, will exit instead of displaying everything since the intention was
+ // to potentially filter some data
+ throw new RuntimeException("Invalid transaction ID format: " + args[i], nfe);
}
}
+ }
- // Parse TStatus filters for print display
- EnumSet<TStatus> filterStatus = null;
- if (cl.hasOption(statusOption.getOpt())) {
- filterStatus = EnumSet.noneOf(TStatus.class);
- String[] tstat = cl.getOptionValues(statusOption.getOpt());
- for (String element : tstat) {
- try {
- filterStatus.add(TStatus.valueOf(element));
- } catch (IllegalArgumentException iae) {
- System.out.printf("Invalid transaction status name: %s%n", element);
- return 1;
- }
+ // Parse TStatus filters for print display
+ EnumSet<TStatus> filterStatus = null;
+ if (printStatus) {
+ filterStatus = EnumSet.noneOf(TStatus.class);
+ String[] tstat = cl.getOptionValues(statusOption.getOpt());
+ for (String element : tstat) {
+ try {
+ filterStatus.add(TStatus.valueOf(element));
+ } catch (IllegalArgumentException iae) {
+ throw new RuntimeException("Invalid transaction status name: " + element, iae);
}
}
+ }
- StringBuilder buf = new StringBuilder(8096);
- Formatter fmt = new Formatter(buf);
- admin.print(zs, zk, context.getZooKeeperRoot() + Constants.ZTABLE_LOCKS, fmt, filterTxid,
- filterStatus);
- shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
- !cl.hasOption(disablePaginationOpt.getOpt()));
- } else if ("dump".equals(cmd)) {
- List<Long> txids;
+ StringBuilder buf = new StringBuilder(8096);
+ Formatter fmt = new Formatter(buf);
+ admin.print(zs, zk, tableLocksPath, fmt, filterTxid, filterStatus);
+ shellState.printLines(Collections.singletonList(buf.toString()).iterator(),
+ !cl.hasOption(disablePaginationOpt.getOpt()));
+ }
- if (args.length == 1) {
- txids = zs.list();
+ private boolean deleteTx(AdminUtil<FateCommand> admin, ZooStore<FateCommand> zs,
+ ZooReaderWriter zk, ServiceLockPath zLockManagerPath, String[] args)
+ throws InterruptedException, KeeperException {
+ boolean success = true;
Review comment:
Fixed in c33b34aef77d4d1dfe9a31eca650983f3f714def
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org