You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maoling (via GitHub)" <gi...@apache.org> on 2024/07/02 17:11:20 UTC
[PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
maoling opened a new pull request, #3394:
URL: https://github.com/apache/cassandra/pull/3394
- more details in the [CASSANDRA-19743](https://issues.apache.org/jira/browse/CASSANDRA-19743)
--
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
Re: [PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3394:
URL: https://github.com/apache/cassandra/pull/3394#discussion_r1986836015
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4209,7 +4209,10 @@ private LinkedHashMap<InetAddressAndPort, Float> getEffectiveOwnership(String ke
for (String keyspaceName : userKeyspaces)
{
if (!Schema.instance.getKeyspaceInstance(keyspaceName).getReplicationStrategy().hasSameSettings(replicationStrategy))
- throw new IllegalStateException("Non-system keyspaces don't have the same replication settings, effective ownership information is meaningless");
+ throw new IllegalStateException(String.format(
+ "Non-system keyspaces: %s and %s don't have the same replication settings, effective ownership information is meaningless",
Review Comment:
@maoling you might probably gather all keyspaces for which replication settings are different and enrich exception message with them?
However, there might be in theory dozens of keyspaces and it might pollute the output unnecessarily. Not sure how to control that.
--
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
Re: [PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3394:
URL: https://github.com/apache/cassandra/pull/3394#discussion_r1986820183
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4184,14 +4184,14 @@ private LinkedHashMap<InetAddressAndPort, Float> getEffectiveOwnership(String ke
if (keyspace != null)
{
if (isLocalSystemKeyspace(keyspace))
- throw new IllegalStateException("Ownership values for keyspaces with LocalStrategy are meaningless");
+ throw new IllegalStateException("Ownership values for keyspaces " + keyspace + " with LocalStrategy are meaningless");
Review Comment:
@maoling I do not think this is necessary. This code path is exercised when `keyspace != null` so a user knows which keyspace it is talking about, no? (Except of that, the constructed message is grammatically incorrect).
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4184,14 +4184,14 @@ private LinkedHashMap<InetAddressAndPort, Float> getEffectiveOwnership(String ke
if (keyspace != null)
{
if (isLocalSystemKeyspace(keyspace))
- throw new IllegalStateException("Ownership values for keyspaces with LocalStrategy are meaningless");
+ throw new IllegalStateException("Ownership values for keyspaces " + keyspace + " with LocalStrategy are meaningless");
KeyspaceMetadata keyspaceInstance = metadata.schema.getKeyspaces().getNullable(keyspace);
if (keyspaceInstance == null)
throw new IllegalArgumentException("The keyspace " + keyspace + ", does not exist");
if (keyspaceInstance.replicationStrategy instanceof LocalStrategy)
- throw new IllegalStateException("Ownership values for keyspaces with LocalStrategy are meaningless");
+ throw new IllegalStateException("Ownership values for keyspaces " + keyspace + " with LocalStrategy are meaningless");
Review Comment:
@maoling same as 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
Re: [PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
Posted by "maoling (via GitHub)" <gi...@apache.org>.
maoling closed pull request #3394: CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging
URL: https://github.com/apache/cassandra/pull/3394
--
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
Re: [PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3394:
URL: https://github.com/apache/cassandra/pull/3394#discussion_r1986820183
##########
src/java/org/apache/cassandra/service/StorageService.java:
##########
@@ -4184,14 +4184,14 @@ private LinkedHashMap<InetAddressAndPort, Float> getEffectiveOwnership(String ke
if (keyspace != null)
{
if (isLocalSystemKeyspace(keyspace))
- throw new IllegalStateException("Ownership values for keyspaces with LocalStrategy are meaningless");
+ throw new IllegalStateException("Ownership values for keyspaces " + keyspace + " with LocalStrategy are meaningless");
Review Comment:
@maoling I do not think this is necessary. This code path is exercised when `keyspace != null` so a user knows which keyspace it is talking about, no?
--
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
Re: [PR] CASSANDRA-19743: add keyspace name to exception message in StorageService#getEffectiveOwnership for better debugging [cassandra]
Posted by "maoling (via GitHub)" <gi...@apache.org>.
maoling commented on PR #3394:
URL: https://github.com/apache/cassandra/pull/3394#issuecomment-2727269840
> However, there might be in theory dozens of keyspaces and it might pollute the output unnecessarily. Not sure how to control that.
You make a valid point. This PR was created last year, and it appears to have no value now. Let me close it. @smiklosovic Thanks for your review.
--
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