You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "bereng (via GitHub)" <gi...@apache.org> on 2023/06/06 11:54:21 UTC
[GitHub] [cassandra] bereng opened a new pull request, #2391: Cassandra 18569 trunk
bereng opened a new pull request, #2391:
URL: https://github.com/apache/cassandra/pull/2391
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] bereng closed pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng closed pull request #2391: Cassandra 18569 trunk
URL: https://github.com/apache/cassandra/pull/2391
--
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] bereng commented on pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#issuecomment-1578612474
Ci is [green](https://app.circleci.com/pipelines/github/bereng/cassandra/974/workflows/0d4cecaf-462a-4051-998b-d8852095d3e6)
--
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] bereng commented on a diff in pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221480525
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -803,6 +805,15 @@ public void testDefaultSslContextFactoryConfiguration()
Assert.assertTrue(config.server_encryption_options.ssl_context_factory.parameters.isEmpty());
}
+ @Test
+ public void testBtiFormatAndStorageCompatibilityMode()
Review Comment:
Yep can do 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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221527092
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
Review Comment:
`UPGRADING` allows testing a subset of the nodes with bti while keeping the other nodes on big. And that composes with other features, like enabling other version-dependant properties and tries on a subset of the nodes.
The original patch for bti was intended to work on the scenario described by `UPGRADING`. What cases do you think might not work on a mixed cluster so that bti should be disallowed until all nodes are upgraded?
Wouldn't that force users to go first to `big-oa` on a first restart with `UPGRADING`, and then to `bti` on a second restart with `NONE`? If bti is supported on `UPGRADING` upgrades could skip the `nc->oa` step.
--
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] bereng commented on a diff in pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1222614180
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
Review Comment:
I really dislike having moving parts while `UPGRADING`. This means we'd have to test the whole codebase in `UPGRADING` which is yet another CI duplication of work. But tries might be a special case being behind a pretty solid feature flag. Also avoiding the extra oa hop while upgrading seems worth it. Lets do it for tries.
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221374492
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -803,6 +805,15 @@ public void testDefaultSslContextFactoryConfiguration()
Assert.assertTrue(config.server_encryption_options.ssl_context_factory.parameters.isEmpty());
}
+ @Test
+ public void testBtiFormatAndStorageCompatibilityMode()
Review Comment:
Shouldn't we test the three possible values of `StorageCompatibilityMode`?
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221361919
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
Review Comment:
Why is BTI rejected with `StorageCompatibilityMode.UPGRADING`?
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221489709
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -803,6 +805,15 @@ public void testDefaultSslContextFactoryConfiguration()
Assert.assertTrue(config.server_encryption_options.ssl_context_factory.parameters.isEmpty());
}
+ @Test
+ public void testBtiFormatAndStorageCompatibilityMode()
Review Comment:
Actually, we can test all compatibility modes against bti and big, and make sure that the test will be updated on new compatibility modes: https://github.com/adelapena/cassandra/blob/30565119702200d7dac215abef33ebc123d8e62c/test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java#L810-L835
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1222919398
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
Review Comment:
That situation, being able to support new features during rolling upgrades, is not new. I don't think that offering the possibility of running in compatibility mode changes it. IMO it doesn't remove the need of having the rolling upgrade dtests that we usually add to prevent multiplexing entire CI. For the scope of this ticket, I think we can assume that tries work in rolling upgrade mode, since that was the assumption when they were merged.
--
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] bereng commented on a diff in pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1222619594
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
+ throw new ConfigurationException(String.format("Selected sstable format '%s' is not available when in storage compatibility mode '%s'.",
Review Comment:
I have addressed the latest 2 comments together. Mixed bag between you and me:
- Yes lets loop all values
- I have moved the validation into SCM. We'll most probably have more validations so having a centralized point, instead of adding and spreading noise to the callers, seems easier to handle. Everything seems tidier and easier to reason about imo.
- I have gone for a straight exception. The added indirection of `validateFeature` seemed unnecessary in this case and I favor the direct call here.
wdyt?
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1222924784
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
+ throw new ConfigurationException(String.format("Selected sstable format '%s' is not available when in storage compatibility mode '%s'.",
Review Comment:
I like the idea of moving the method to `StorageCompatibilityMode`. That creates coupling between `StorageCompatibilityMode` and the sstable format, but that coupling allows to clearly see and centralize all dependencies. It also nicely hides the fact that `DatabaseDescriptor. validateWriteFormatVsStorageCompatibilityMode` was a separate method just to allow unit testing. The point of using a generic method on `StorageCompatibilityMode` was guaranteeing that all similar messages have the same format, like ending in `is not available when in storage compatibility mode '%s'`. We can add back that generic message formatting when we have more than one caller, adding it now is probably overkill.
--
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 #2391: Cassandra 18569 trunk
Posted by "adelapena (via GitHub)" <gi...@apache.org>.
adelapena commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221400438
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
+ throw new ConfigurationException(String.format("Selected sstable format '%s' is not available when in storage compatibility mode '%s'.",
Review Comment:
Perhaps we could take a guardrail-ish approach and delegate the formatting of the message to `StorageCompatibilityMode`. Something like: https://github.com/adelapena/cassandra/commit/30119694b64beb09d354984738ebc74fb447f7a9
That way we can make sure that all compatibility validation exceptions have a similar formatting. wdyt?
--
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] bereng commented on a diff in pull request #2391: Cassandra 18569 trunk
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2391:
URL: https://github.com/apache/cassandra/pull/2391#discussion_r1221480184
##########
src/java/org/apache/cassandra/config/DatabaseDescriptor.java:
##########
@@ -1424,9 +1425,20 @@ private static void validateSSTableFormatFactories(Iterable<SSTableFormat.Factor
if (selectedFormat == null)
throw new ConfigurationException(String.format("Selected sstable format '%s' is not available.", selectedFormatName));
+ validateWriteFormatVsStorageCompatibilityMode(selectedFormat, getStorageCompatibilityMode());
+
return selectedFormat;
}
+ @VisibleForTesting
+ public static void validateWriteFormatVsStorageCompatibilityMode(SSTableFormat<?, ?> selectedFormat, StorageCompatibilityMode mode)
+ {
+ if (selectedFormat.name().equals(BtiFormat.NAME) && mode != StorageCompatibilityMode.NONE)
Review Comment:
To be on the safe side and not muddy the waters further while on mixed compatibility mode clusters. That would just complicate things further for no/little benefit imo.
--
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