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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
