You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2023/06/07 12:04:39 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request, #2394: CASSANDRA-18572
smiklosovic opened a new pull request, #2394:
URL: https://github.com/apache/cassandra/pull/2394
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222140672
##########
src/java/org/apache/cassandra/tools/NodeTool.java:
##########
@@ -281,6 +286,11 @@ public int execute(String... args)
return status;
}
+ public void executeWithProbe(NodeProbe probe, Runnable r)
Review Comment:
this will be overridden in Instance
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222140423
##########
src/java/org/apache/cassandra/tools/NodeTool.java:
##########
@@ -353,11 +363,13 @@ public static abstract class NodeToolCmd implements NodeToolCmdRunnable
protected boolean printPort = false;
private INodeProbeFactory nodeProbeFactory;
+ private NodeTool nodeTool;
protected Output output;
@Override
- public void run(INodeProbeFactory nodeProbeFactory, Output output)
+ public void run(NodeTool nodeTool, INodeProbeFactory nodeProbeFactory, Output output)
Review Comment:
I had to pass the reference to nodeTool here because I need to get a hold of NodeProbe from it later on in `Instance` to execute additional logic.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222138826
##########
src/java/org/apache/cassandra/tools/NodeTool.java:
##########
@@ -442,7 +458,7 @@ private NodeProbe connect()
} catch (IOException | SecurityException e)
{
Throwable rootCause = Throwables.getRootCause(e);
- output.err.println(format("nodetool: Failed to connect to '%s:%s' - %s: '%s'.", host, port, rootCause.getClass().getSimpleName(), rootCause.getMessage()));
Review Comment:
one import less!
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222136451
##########
test/distributed/org/apache/cassandra/distributed/impl/InternalNodeProbeFactory.java:
##########
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-package org.apache.cassandra.distributed.mock.nodetool;
Review Comment:
I moved the classes out of `mock/nodetool` package to impl, there is no mocking anymore so it is not relevant anymore either
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222154917
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1085,22 +1083,22 @@ public Throwable getLatestError()
return latestError;
}
- public int execute(String... args)
+ @Override
+ public void executeWithProbe(NodeProbe probe, Runnable r)
{
try
{
- return super.execute(args);
+ if (withNotifications)
+ probe.getStorageService().addNotificationListener(notifications, null, null);
Review Comment:
There is possibility to have or not have _notifications_ from StorageService. The original code was dealing with it.
So here, we need to cover both cases when we are using `InternalNodeProbeFactory` as well as `JMXAwareInternalNodeProbeFactory` which will interact with a proper StorageServiceMBean - it will not call that instance directly as in case of InternalNodeProbeFactory .
Also, I need to get a hold of `probe` to call `getStorageService` on. `NodeProbe` is not intialized in `NodeTool` but later in `NodeToolCmd.runInternal`
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222141371
##########
src/java/org/apache/cassandra/tools/NodeProbe.java:
##########
@@ -288,10 +293,18 @@ protected void connect() throws IOException
"Invalid ObjectName? Please report this as a bug.", e);
}
- memProxy = ManagementFactory.newPlatformMXBeanProxy(mbeanServerConn,
- ManagementFactory.MEMORY_MXBEAN_NAME, MemoryMXBean.class);
- runtimeProxy = ManagementFactory.newPlatformMXBeanProxy(
- mbeanServerConn, ManagementFactory.RUNTIME_MXBEAN_NAME, RuntimeMXBean.class);
+ try
+ {
Review Comment:
these are not available when connecting via config / JMXUtils
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222138175
##########
src/java/org/apache/cassandra/tools/nodetool/Info.java:
##########
@@ -59,14 +59,29 @@ public void execute(NodeProbe probe)
out.printf("%-23s: %s%n", "Generation No", 0);
// Uptime
- long secondsUp = probe.getUptime() / 1000;
- out.printf("%-23s: %d%n", "Uptime (seconds)", secondsUp);
+ try
Review Comment:
these try blocks are here because when tested with JMX feature, Nodetool will not find Memory bean on Instance (weird, right?), same for heap memory usage. Basically when it tries to connect to JMX and it initialises mbeans, it will not find Memory and the one below.
For testing purposes, the output in catch is just enough.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222593961
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1062,17 +1060,17 @@ public void close()
public static class DTestNodeTool extends NodeTool implements AutoCloseable
{
- private final StorageServiceMBean storageProxy;
private final CollectingNotificationListener notifications = new CollectingNotificationListener();
- private final InternalNodeProbe internalNodeProbe;
+ private final boolean withNotifications;
private Throwable latestError;
+ private NodeProbe probe;
- public DTestNodeTool(boolean withNotifications, Output output)
+ public DTestNodeTool(IInstanceConfig config, boolean withNotifications, Output output)
{
- super(new InternalNodeProbeFactory(withNotifications), output);
Review Comment:
it seems to me like we were actually using two node probes before, huh?
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222154917
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1085,22 +1083,22 @@ public Throwable getLatestError()
return latestError;
}
- public int execute(String... args)
+ @Override
+ public void executeWithProbe(NodeProbe probe, Runnable r)
{
try
{
- return super.execute(args);
+ if (withNotifications)
+ probe.getStorageService().addNotificationListener(notifications, null, null);
Review Comment:
There is possibility to have or not have _notifications_ from StorageService. The original code was dealing with it.
So here, we need to cover both cases when we are using `InternalNodeProbeFactory` as well as `JMXAwareInternalNodeProbeFactory` which will interact with a proper StorageServiceMBean - it will not call that instance directly as in case of InternalNodeProbeFactory .
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222138175
##########
src/java/org/apache/cassandra/tools/nodetool/Info.java:
##########
@@ -59,14 +59,29 @@ public void execute(NodeProbe probe)
out.printf("%-23s: %s%n", "Generation No", 0);
// Uptime
- long secondsUp = probe.getUptime() / 1000;
- out.printf("%-23s: %d%n", "Uptime (seconds)", secondsUp);
+ try
Review Comment:
these try blocks are here because when tested with JMX feature, Nodetool will not find Memory bean on Instance (weird, right?), same for heap memory usage. Basically when it tries to connect to JMX and it initialises mbeans, it will not find Memory and the one below.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2394: CASSANDRA-18572
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2394:
URL: https://github.com/apache/cassandra/pull/2394#discussion_r1222139261
##########
src/java/org/apache/cassandra/tools/NodeTool.java:
##########
@@ -375,7 +387,11 @@ public void runInternal()
try (NodeProbe probe = connect())
{
- execute(probe);
+ if (nodeTool != null)
Review Comment:
there are tools like Sjk which are executing this differently and not everytime nodetool is present.
--
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-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org