You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2023/01/13 11:00:46 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request, #2095: CASSANDRA-18152
smiklosovic opened a new pull request, #2095:
URL: https://github.com/apache/cassandra/pull/2095
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070139850
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
@adelapena I understand, but whatever it was set to, it not it always safe to set it to false anyway? We are in the closing method. That is probably the last method which will be used in this class.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070184622
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1018,15 +1018,18 @@ public void close()
}
}
- public static class DTestNodeTool extends NodeTool {
+ public static class DTestNodeTool extends NodeTool implements AutoCloseable
+ {
private final StorageServiceMBean storageProxy;
private final CollectingNotificationListener notifications = new CollectingNotificationListener();
-
+ private final InternalNodeProbe internalNodeProbe;
private Throwable latestError;
- public DTestNodeTool(boolean withNotifications, Output output) {
+ public DTestNodeTool(boolean withNotifications, Output output)
+ {
super(new InternalNodeProbeFactory(withNotifications), output);
- storageProxy = new InternalNodeProbe(withNotifications).getStorageService();
+ internalNodeProbe = new InternalNodeProbe(withNotifications);
Review Comment:
we cache the property so we can call close on that which sets it back to false
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for some dtests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
Also, the way it is currently done, like `public static StorageService instance = new StorageService()`, I do not see how we could pass it into the constructor without changing every place where / when StorageService is instantiated / accessed for the first time.
Maybe not so obvious fact is that the way I see it is that StorageService is instantiated _every time_ for each dtest.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for tests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070139850
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
@adelapena I understand, but whatever it was set to, is not it always safe to set it to false anyway? We are in the closing method. That is probably the last method which will be used in this class.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070139850
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
@adelapena I understand, but whatever it was set to, is not it always safe to set it to false anyway? We are in the closing method. That is probably the last method which will be used in this class after which it is not safe to use it anymore.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for tests) during whole lifetime of that object.
--
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] adelapena commented on a diff in pull request #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1069993322
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -331,8 +331,13 @@
*
* If only keyspaces are specified, mutations for all tables in such keyspace will be replayed
* */
- COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null)
+ COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null),
+ /**
+ * When true, removeNotificationListener and addNotificationListener methods on StorageService will be NO-OP.
+ * Defaults to false.
+ */
+ STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS("cassandra.storage.skip.notification.listeners", "false");
Review Comment:
Nit: extra semicolon at the end of the line
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -331,8 +331,13 @@
*
* If only keyspaces are specified, mutations for all tables in such keyspace will be replayed
* */
- COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null)
+ COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null),
+ /**
+ * When true, removeNotificationListener and addNotificationListener methods on StorageService will be NO-OP.
Review Comment:
Maybe we could extend the comment to mention that this is used by dtests. Actually, I don't know if there is a use case for setting this as a JVM property. If not, would it make sense to make this just a mutable property in `StorageService`?
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
This assumes that `STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS` was false before the call to `connect`. Maybe we can save the original value of that property on the call to `connect`, and restore it here.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -7134,4 +7136,26 @@ public boolean getSkipStreamDiskSpaceCheck()
{
return DatabaseDescriptor.getSkipStreamDiskSpaceCheck();
}
+
+ @Override
+ public void removeNotificationListener(NotificationListener listener) throws ListenerNotFoundException
+ {
+ if (!skipNotificationListeners)
+ super.removeNotificationListener(listener);
+ }
+
+ @Override
+ public void removeNotificationListener(NotificationListener listener, NotificationFilter filter, Object handback) throws ListenerNotFoundException
+ {
+ if (!skipNotificationListeners)
+ super.removeNotificationListener(listener, filter, handback);
+ }
+
+ @Override
+ public void addNotificationListener(NotificationListener listener,
+ NotificationFilter filter,
+ Object handback) throws java.lang.IllegalArgumentException {
Review Comment:
Nit: misplaced bracket at the end of the line.
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
`InternalNodeProbe` dynamically changes the value of `STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS`. However, I understand that the `final` value of `skipNotificationListeners` wouldn't see those changes, just the initial value.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070143458
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -331,8 +331,13 @@
*
* If only keyspaces are specified, mutations for all tables in such keyspace will be replayed
* */
- COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null)
+ COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null),
+ /**
+ * When true, removeNotificationListener and addNotificationListener methods on StorageService will be NO-OP.
Review Comment:
Interesting idea. I have to sleep on it but I dont object it in general.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for some dtests tests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070143458
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -331,8 +331,13 @@
*
* If only keyspaces are specified, mutations for all tables in such keyspace will be replayed
* */
- COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null)
+ COMMIT_LOG_REPLAY_LIST("cassandra.replayList", null),
+ /**
+ * When true, removeNotificationListener and addNotificationListener methods on StorageService will be NO-OP.
Review Comment:
answered 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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070139850
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
I understand, but whatever it was set to, it not it always safe to set it to false anyway? We are in the closing method. That is probably the last method which will be used in this class.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070184285
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -66,26 +63,9 @@ protected void connect()
mbeanServerConn = null;
jmxc = null;
- if (withNotifications)
- {
- ssProxy = StorageService.instance;
- }
- else
- {
- // replace the notification apis with a no-op method
- StorageServiceMBean mock = Mockito.spy(StorageService.instance);
- Mockito.doNothing().when(mock).addNotificationListener(Mockito.any(), Mockito.any(), Mockito.any());
- try
- {
- Mockito.doNothing().when(mock).removeNotificationListener(Mockito.any(), Mockito.any(), Mockito.any());
- Mockito.doNothing().when(mock).removeNotificationListener(Mockito.any());
- }
- catch (ListenerNotFoundException e)
- {
- throw new AssertionError(e);
- }
- ssProxy = mock;
- }
+ StorageService.instance.skipNotificationListeners = !withNotifications;
Review Comment:
pretty simple, right? then we change it back to default (false) in close method.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena I had quite long answer prepared but I need to rethink the whole thing. Thanks for the feedback.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070139850
##########
test/distributed/org/apache/cassandra/distributed/mock/nodetool/InternalNodeProbe.java:
##########
@@ -105,7 +88,8 @@ protected void connect()
@Override
public void close()
{
- // nothing to close. no-op
+ if (!withNotifications)
+ CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.setBoolean(false);
Review Comment:
@adelapena answered 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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for some dtests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
Also, the way it is currently done, like `public static StorageService instance = new StorageService()`, I do not see how we could pass it into the constructor without changing every place where / when StorageService is instantiated.
Maybe not so obvious fact is that the way I see it is that StorageService is instantiated _every time_ for each dtest.
--
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 closed pull request #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #2095: CASSANDRA-18152
URL: https://github.com/apache/cassandra/pull/2095
--
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] adelapena commented on a diff in pull request #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1071144189
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ public volatile boolean skipNotificationListeners = false;
Review Comment:
I'd add a brief comment saying that this is used only by dtests, and maybe reference the ticket. Maybe we should mark it as `@VisibleForTesting`?
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for some dtests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
Maybe not so obvious fact is that the way I see it is that StorageService is instantiated _every time_ for each dtest.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable. And I do not want that to be mutable. Why should it be mutable? What value it has, in runtime, to be able to disable or enable it once StorageService object (effectively singleton) is instantiated? You either want it to be turned on (in production) or off (for some dtests) during whole lifetime of that object.
We can abandon system property approach if the same is effectively achieved by some property on StorageService but I do not want that to be mutable and it seems to me that having a system property is the most straighforward way how to "communicate" this fact of enabling / disabling.
If I am about to set it _after_ StorageService object is created, that field has to be mutable (not final) or pass it there via constructor. As I explained, I do not see any value nor reason in mutability.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070145635
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -231,6 +231,8 @@ private static int getSchemaDelay()
private final SamplingManager samplingManager = new SamplingManager();
+ private final boolean skipNotificationListeners = CassandraRelevantProperties.STORAGE_SERVICE_SKIP_NOTIFICATION_LISTENERS.getBoolean();
Review Comment:
@adelapena the logic is that I set that property _before_ StorageService is instantiated. So this will be just mere reader of that value. It is not meant to be mutable.
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070184460
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1018,15 +1018,18 @@ public void close()
}
}
- public static class DTestNodeTool extends NodeTool {
+ public static class DTestNodeTool extends NodeTool implements AutoCloseable
Review Comment:
this implements AutoCloseable so we can use it in try-with-resources in Instance above
--
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 #2095: CASSANDRA-18152
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2095:
URL: https://github.com/apache/cassandra/pull/2095#discussion_r1070184325
##########
test/distributed/org/apache/cassandra/distributed/impl/Instance.java:
##########
@@ -1072,6 +1075,12 @@ protected void err(Throwable e)
super.err(e);
latestError = e;
}
+
+ @Override
+ public void close()
+ {
+ internalNodeProbe.close();
Review Comment:
here we set it to false
--
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