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/11/08 08:33:40 UTC
[PR] Cassandra-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
bereng opened a new pull request, #2880:
URL: https://github.com/apache/cassandra/pull/2880
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
Re: [PR] Cassandra-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387511320
##########
test/unit/org/apache/cassandra/io/sstable/LegacySSTableTest.java:
##########
@@ -101,7 +101,7 @@ public class LegacySSTableTest
// Get all versions up to the current one. Useful for testing in compatibility mode C18301
private static String[] getValidLegacyVersions()
{
- String[] versions = {"oa", "da", "nc", "nb", "na", "me", "md", "mc", "mb", "ma"};
+ String[] versions = {"oa", "da", "nb", "na", "me", "md", "mc", "mb", "ma"};
return Arrays.stream(versions).filter((v) -> v.compareTo(BigFormat.getInstance().getLatestVersion().toString()) <= 0).toArray(String[]::new);
Review Comment:
`oa` needs to be filtered out unless you do `test-oa`. So in compatibility mode you can go up to nb, out of it you can go up to oa.
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#issuecomment-1801386848
Ci [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/1100/workflows/d4356670-cfff-411b-9fde-78aed266862c)
CI [j17](https://app.circleci.com/pipelines/github/bereng/cassandra/1100/workflows/edf84b12-afd8-4ebe-917b-966f554482fe)
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#issuecomment-1801432774
Note to self: Remove `test/data/legacy-sstables/nc`
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1386252646
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -458,7 +458,7 @@ protected SSTableReader(Builder<?, ?> builder, Owner owner)
this.openReason = builder.getOpenReason();
this.first = builder.getFirst();
this.last = builder.getLast();
- this.bounds = AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
+ this.bounds = first == null || last == null || AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
Review Comment:
@jacek-lewandowski does this make sense to you? Ping me if you need more context
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1389125436
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -458,7 +458,7 @@ protected SSTableReader(Builder<?, ?> builder, Owner owner)
this.openReason = builder.getOpenReason();
this.first = builder.getFirst();
this.last = builder.getLast();
- this.bounds = AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
+ this.bounds = first == null || last == null || AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
Review Comment:
Just for future context: DTest scrub_test.TestScrub.test_standalone_scrub_essential_files_only exercises a path where we scrub sstables where some of the sstable files are missing. This can make those vars null so it's a rare but valid code path.
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387514028
##########
test/unit/org/apache/cassandra/io/sstable/metadata/MetadataSerializerTest.java:
##########
@@ -85,7 +86,9 @@ public void testSerialization() throws IOException
for (MetadataType type : MetadataType.values())
{
- assertEquals(originalMetadata.get(type), deserialized.get(type));
+ if ((type != MetadataType.STATS) || latestVersion.hasImprovedMinMax())
Review Comment:
It's the same problem as before where `first` and `last` are null for `nb`. Let's solve that underlying issue first and then we'll know if we still need this.
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387854984
##########
test/unit/org/apache/cassandra/io/sstable/metadata/MetadataSerializerTest.java:
##########
@@ -85,7 +86,9 @@ public void testSerialization() throws IOException
for (MetadataType type : MetadataType.values())
{
- assertEquals(originalMetadata.get(type), deserialized.get(type));
+ if ((type != MetadataType.STATS) || latestVersion.hasImprovedMinMax())
Review Comment:
got it, it is another consequence of the fact that getLatestVersion does not return the latest version
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#issuecomment-1803635579
+1, the remaining issues are to be discussed in https://issues.apache.org/jira/browse/CASSANDRA-19012
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387852859
##########
test/unit/org/apache/cassandra/io/sstable/LegacySSTableTest.java:
##########
@@ -101,7 +101,7 @@ public class LegacySSTableTest
// Get all versions up to the current one. Useful for testing in compatibility mode C18301
private static String[] getValidLegacyVersions()
{
- String[] versions = {"oa", "da", "nc", "nb", "na", "me", "md", "mc", "mb", "ma"};
+ String[] versions = {"oa", "da", "nb", "na", "me", "md", "mc", "mb", "ma"};
return Arrays.stream(versions).filter((v) -> v.compareTo(BigFormat.getInstance().getLatestVersion().toString()) <= 0).toArray(String[]::new);
Review Comment:
so it is just because we cannot read oa sstables
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387129592
##########
test/unit/org/apache/cassandra/io/sstable/LegacySSTableTest.java:
##########
@@ -101,7 +101,7 @@ public class LegacySSTableTest
// Get all versions up to the current one. Useful for testing in compatibility mode C18301
private static String[] getValidLegacyVersions()
{
- String[] versions = {"oa", "da", "nc", "nb", "na", "me", "md", "mc", "mb", "ma"};
+ String[] versions = {"oa", "da", "nb", "na", "me", "md", "mc", "mb", "ma"};
return Arrays.stream(versions).filter((v) -> v.compareTo(BigFormat.getInstance().getLatestVersion().toString()) <= 0).toArray(String[]::new);
Review Comment:
btw. why is this filtering? It seems like it does not filter anything. Moreover if feels incorrect as it assumes `BigFormat`
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387849454
##########
src/java/org/apache/cassandra/io/sstable/format/SSTableReader.java:
##########
@@ -458,7 +458,7 @@ protected SSTableReader(Builder<?, ?> builder, Owner owner)
this.openReason = builder.getOpenReason();
this.first = builder.getFirst();
this.last = builder.getLast();
- this.bounds = AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
+ this.bounds = first == null || last == null || AbstractBounds.strictlyWrapsAround(first.getToken(), last.getToken())
Review Comment:
ok, this is valid +1
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "jacek-lewandowski (via GitHub)" <gi...@apache.org>.
jacek-lewandowski commented on code in PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#discussion_r1387122443
##########
test/unit/org/apache/cassandra/io/sstable/metadata/MetadataSerializerTest.java:
##########
@@ -85,7 +86,9 @@ public void testSerialization() throws IOException
for (MetadataType type : MetadataType.values())
{
- assertEquals(originalMetadata.get(type), deserialized.get(type));
+ if ((type != MetadataType.STATS) || latestVersion.hasImprovedMinMax())
Review Comment:
is it needed?
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng commented on PR #2880:
URL: https://github.com/apache/cassandra/pull/2880#issuecomment-1805358817
Rebased and squashed [j11](https://app.circleci.com/pipelines/github/bereng/cassandra/1105/workflows/e557a641-d861-45af-91ee-f3b451d385ab) and [j17](https://app.circleci.com/pipelines/github/bereng/cassandra/1105/workflows/ed49ce78-e9dd-4b19-bbd4-47ba8255537c). Only an unrelated Circle env problem
--
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-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa' [cassandra]
Posted by "bereng (via GitHub)" <gi...@apache.org>.
bereng closed pull request #2880: Cassandra-19010 Remove sstable format 'nc' and make 5.0 'nb' and 'oa'
URL: https://github.com/apache/cassandra/pull/2880
--
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