azotcsit commented on a change in pull request #1232:
URL: https://github.com/apache/cassandra/pull/1232#discussion_r718500400
##########
File path: src/java/org/apache/cassandra/tools/NodeTool.java
##########
@@ -195,6 +195,7 @@ public int execute(String... args)
SetInterDCStreamThroughput.class,
SetLoggingLevel.class,
SetMaxHintWindow.class,
+ ListEndpointsPendingHints.class,
Review comment:
I feel the classes are sorted alphabetically. Could you please put it to
a proper place.
##########
File path: src/java/org/apache/cassandra/tools/NodeProbe.java
##########
@@ -1140,6 +1146,11 @@ public void truncateHints()
hsProxy.deleteAllHints();
}
+ public List<PendingHintsInfo> listPendingHints()
Review comment:
I saw you made `PendingHintsInfo` serializable but I'm not sure how all
this stuff works if you connected via JMX directly. Could you please confirm
that the current implementation works through both nodetool and JMX.
My general understanding was that we prefer returning basic objects (lists,
maps, strings, etc) from MBeans and NodeProbe instead of custom types, but I'm
not sure. I'll let @ekaterinadimitrova2 to comment here.
##########
File path: src/java/org/apache/cassandra/hints/HintsServiceMBean.java
##########
@@ -40,4 +42,11 @@
* being dispatched right now, or being written to).
*/
void deleteAllHintsForEndpoint(String address);
+
+ /**
+ * Lists all the endpoints that this node has hints for.
+ *
+ * @return a list of endpoints with relevant hint information - total
number of files, newest and oldest timestamps.
+ */
+ List<PendingHintsInfo> listPendingHints();
Review comment:
probably I'd rename it to `getPendingHintsInfo` or `getAllPendingHints`.
WDYT?
the javadoc is not correct anymore, right? I'd say "Returns all pending
hints that this node has."
the same changes need to be addressed in `HintsService`.
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/ListEndpointsPendingHints.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.cassandra.tools.nodetool;
+
+import java.net.UnknownHostException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import io.airlift.airline.Command;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.EndpointSnitchInfoMBean;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.NodeTool;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+
+@Command(name = "listendpointspendinghints", description = "Print all the
endpoints that this node has hints for")
Review comment:
And WRT this I feel "listpendinghints" would be more reasonable name.
WDYT?
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/ListEndpointsPendingHints.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.cassandra.tools.nodetool;
+
+import java.net.UnknownHostException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import io.airlift.airline.Command;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.EndpointSnitchInfoMBean;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.NodeTool;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+
+@Command(name = "listendpointspendinghints", description = "Print all the
endpoints that this node has hints for")
Review comment:
> Print all the endpoints that this node has hints for
This is not correct, right? As we discussed it does not print hints per
target node or print details for multiple nodes. It just prints the details for
current node.
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/ListEndpointsPendingHints.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.cassandra.tools.nodetool;
+
+import java.net.UnknownHostException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import io.airlift.airline.Command;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.EndpointSnitchInfoMBean;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.NodeTool;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+
+@Command(name = "listendpointspendinghints", description = "Print all the
endpoints that this node has hints for")
+public class ListEndpointsPendingHints extends NodeTool.NodeToolCmd
+{
+ @Override
+ public void execute(NodeProbe probe)
+ {
+ List<PendingHintsInfo> pendingHints = probe.listPendingHints();
+ if(pendingHints.isEmpty())
+ {
+ probe.output().out.println("This node does not have hints for
other endpoints");
+ }
+ else
+ {
+ Map<String, String> endpointMap =
probe.getHostIdToEndpointWithPort();
+ Map<String, String> simpleStates = probe.getSimpleStatesWithPort();
+ EndpointSnitchInfoMBean epSnitchInfo =
probe.getEndpointSnitchInfoProxy();
+
+ SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd
HH:mm:ss,SSS");
Review comment:
I haven't seen comma delimiter for milliseconds previously, usually it
is like "yyyy-MM-dd HH:mm:ss.SSS" (dot instead of comma). Check examples at
https://docs.oracle.com/javase/6/docs/api/java/text/SimpleDateFormat.html.
What timezone the dates will be printed at? I feel we need to print the
timezone ("X" pattern) itself and also we need to decide whether we want to see
local or UTC timezone in the output. For me UTC makes more sense because
theoretically nodes may have different zones and the mesh (as Brandon mentioned
in the ticket) may take the output from many servers.
nit: I'd suggest to use `DateTimeFormatter` and modern Java 8 classes, they
seem to be more convenient to use and they force a user to to raise timezone
questions (unlikely to to old class that silently rely on the local timezone).
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/ListEndpointsPendingHints.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.cassandra.tools.nodetool;
+
+import java.net.UnknownHostException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import io.airlift.airline.Command;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.EndpointSnitchInfoMBean;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.NodeTool;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+
+@Command(name = "listendpointspendinghints", description = "Print all the
endpoints that this node has hints for")
+public class ListEndpointsPendingHints extends NodeTool.NodeToolCmd
Review comment:
I feel once we have a test for the table, probably this one will be
easy, but again I'm not yet sure how to create it at the moment.
##########
File path: src/java/org/apache/cassandra/hints/HintsStore.java
##########
@@ -100,6 +101,22 @@ InetAddressAndPort address()
return StorageService.instance.getEndpointForHostId(hostId);
}
+ PendingHintsInfo getPendingHintsInfo()
Review comment:
it would be nice to add a corresponding unit test in `HintsStoreTest`.
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/ListEndpointsPendingHints.java
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.cassandra.tools.nodetool;
+
+import java.net.UnknownHostException;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import io.airlift.airline.Command;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.EndpointSnitchInfoMBean;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.tools.NodeProbe;
+import org.apache.cassandra.tools.NodeTool;
+import org.apache.cassandra.tools.nodetool.formatter.TableBuilder;
+
+@Command(name = "listendpointspendinghints", description = "Print all the
endpoints that this node has hints for")
+public class ListEndpointsPendingHints extends NodeTool.NodeToolCmd
+{
+ @Override
+ public void execute(NodeProbe probe)
+ {
+ List<PendingHintsInfo> pendingHints = probe.listPendingHints();
+ if(pendingHints.isEmpty())
+ {
+ probe.output().out.println("This node does not have hints for
other endpoints");
+ }
+ else
+ {
+ Map<String, String> endpointMap =
probe.getHostIdToEndpointWithPort();
+ Map<String, String> simpleStates = probe.getSimpleStatesWithPort();
+ EndpointSnitchInfoMBean epSnitchInfo =
probe.getEndpointSnitchInfoProxy();
+
+ SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd
HH:mm:ss,SSS");
Review comment:
I don't really know how TimestampType columns int the VT's output would
look like, but probably it makes sense to use the same format/timezone here.
Just for unification purpose.
##########
File path: src/java/org/apache/cassandra/db/virtual/PendingHintsTable.java
##########
@@ -0,0 +1,115 @@
+/*
+ * 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.cassandra.db.virtual;
+
+import java.net.InetAddress;
+import java.util.Collections;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.marshal.InetAddressType;
+import org.apache.cassandra.db.marshal.Int32Type;
+import org.apache.cassandra.db.marshal.TimestampType;
+import org.apache.cassandra.db.marshal.UTF8Type;
+import org.apache.cassandra.db.marshal.UUIDType;
+import org.apache.cassandra.dht.LocalPartitioner;
+import org.apache.cassandra.gms.FailureDetector;
+import org.apache.cassandra.gms.FailureDetectorMBean;
+import org.apache.cassandra.hints.HintsService;
+import org.apache.cassandra.hints.PendingHintsInfo;
+import org.apache.cassandra.locator.IEndpointSnitch;
+import org.apache.cassandra.locator.InetAddressAndPort;
+import org.apache.cassandra.schema.TableMetadata;
+import org.apache.cassandra.service.StorageService;
+
+final class PendingHintsTable extends AbstractVirtualTable
Review comment:
I'm not yet really sure how to create a test for this table. I'll be
working on a similar (for running compactions) in the scope of CASSANDRA-16976,
so I'll share it once done.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
