dcapwell commented on code in PR #3842:
URL: https://github.com/apache/cassandra/pull/3842#discussion_r1934177395
##########
test/distributed/org/apache/cassandra/distributed/test/accord/AccordDropKeyspaceTest.java:
##########
@@ -35,8 +34,7 @@ public void dropKeyspace() throws IOException
int steps = 5;
try (Cluster cluster = Cluster.build(3)
.withoutVNodes()
- .withConfig(c -> c.with(Feature.values())
Review Comment:
please revert, `Feature.values()` is 100% intentional
##########
test/distributed/org/apache/cassandra/distributed/test/accord/AccordDropTableTest.java:
##########
@@ -35,8 +34,7 @@ public void dropTable() throws IOException
int steps = 5;
try (Cluster cluster = Cluster.build(3)
.withoutVNodes()
- .withConfig(c -> c.with(Feature.values())
Review Comment:
please revert, `Feature.values()` is 100% intentional
##########
src/java/org/apache/cassandra/service/accord/FetchTopology.java:
##########
@@ -37,6 +41,7 @@
public class FetchTopology
{
+ private static final Logger log =
LoggerFactory.getLogger(FetchTopology.class);
Review Comment:
nit, in this project we always use `logger`
```
$ grep -r 'Logger log' src/ | awk -F: '{print $2}' | awk -F= '{print $1}' |
sort | uniq -c | sort -k1,1 -h
1 Logger logBackLogger
1 Logger logbackLogger
1 private static Logger logger
1 public static void logInitializationOutcome(Logger logger)
1 private boolean hasAppenders(Logger logBackLogger)
1 private static Logger logger()
1 private static final Logger logger;
1 private static final org.slf4j.Logger logger
1 protected static final NoSpamLogger logger
1 public static <K, V> void dumpDiff(Logger logger, Map<K, V> l,
Map<K, V> r)
1 public static Logger logger
1 public static NoSpamLogStatement getStatement(Logger logger, String
message, long minInterval, TimeUnit unit)
1 public static NoSpamLogger getLogger(Logger logger, long
minInterval, TimeUnit unit)
1 public static boolean log(Logger logger, Level level, String key,
long minInterval, TimeUnit unit, String message, Object... objects)
1 public static boolean log(Logger logger, Level level, String key,
long minInterval, TimeUnit unit, String message, Supplier<Object[]> objects)
1 public static boolean log(Logger logger, Level level, String key,
long minInterval, TimeUnit unit, long nowNanos, String message, Object...
objects)
1 public static boolean log(Logger logger, Level level, String key,
long minInterval, TimeUnit unit, long nowNanos, String message,
Supplier<Object[]> objects)
1 public static boolean log(Logger logger, Level level, long
minInterval, TimeUnit unit, String message, Object... objects)
1 public static boolean log(Logger logger, Level level, long
minInterval, TimeUnit unit, String message, Supplier<Object[]> objects)
1 public static void dumpDiff(Logger logger, Map<ReplicationParams,
DataPlacement> l, Map<ReplicationParams, DataPlacement> r)
1 public static void logSystemInfo(Logger logger)
1 public void log(Logger log)
1 public void logRowCountPerLeaf(Logger logger)
1 public void logRowSizePerLeaf(Logger logger)
2 protected final static Logger logger
3 for (Logger logBackLogger
3 private final Logger logger
3 private static Logger logger
6 static final Logger logger
7 Logger logger
11 private final static Logger logger
19 private static final Logger logger
26 protected static final Logger logger
464 private static final Logger logger
```
##########
src/java/org/apache/cassandra/service/accord/FetchMinEpoch.java:
##########
@@ -53,40 +50,32 @@
// TODO (required, efficiency): this can be simplified: we seem to always use
"entire range"
public class FetchMinEpoch
{
+ private static final FetchMinEpoch instance = new FetchMinEpoch();
Review Comment:
nit, we could use `NoPayload` instead
##########
src/java/org/apache/cassandra/service/accord/FetchTopology.java:
##########
@@ -67,24 +72,17 @@ public FetchTopology(long epoch)
public static class Response
{
- private static Response UNKNOWN = new Response(-1, null) {
- public String toString()
- {
- return "UNKNOWN_TOPOLOGY{}";
- }
- };
+ private static Response unkonwn(long epoch)
Review Comment:
`unknown`?
##########
src/java/org/apache/cassandra/service/accord/FetchTopology.java:
##########
@@ -123,18 +116,20 @@ public Response(long epoch, Topology topology)
long epoch = message.payload.epoch;
Topology topology =
AccordService.instance().topology().maybeGlobalForEpoch(epoch);
if (topology == null)
- MessagingService.instance().respond(Response.UNKNOWN, message);
+ MessagingService.instance().respond(Response.unkonwn(epoch),
message);
Review Comment:
im confused by this line
```
private static Response unkonwn(long epoch)
{
throw new IllegalStateException("Unknown topology: " + epoch);
}
```
`Response.unkonwn` only throws, so this `response` won't do anything as that
method won't be called
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
