CNDB-15745: Supports immutable components for SAI AA version #2079
Conversation
Checklist before you submit for review
|
pcmanus
force-pushed
the
CNDB-15745-immutable-components-in-AA
branch
from
69965fa to
70fb738
Compare
pkolaczk
left a comment
pkolaczk
left a comment
There was a problem hiding this comment.
Looks good to me.
I have two nitpicks about the code that was already there, but is nearby. Up to you if you want to improve it.
| // Allow restoring legacy behavior of deleting sai components before a rebuild (which implies a rebuild cannot be | ||
| // done without first stopping reads on that index) | ||
| IMMUTABLE_SAI_COMPONENTS("cassandra.sai.immutable_components", "false"), |
There was a problem hiding this comment.
Side note: This comment is not very clear explaining whether the property should be set to true or false to restore the legacy behavior. I believe if the property is false then the behavior is legacy because ISC are disabled, correct?
There was a problem hiding this comment.
You're right, both that it's a bit confusing and that the default, false, is the legacy. I've amended the comment.
| int generation = 0; | ||
| String maybeGenerationStr = componentStr.substring(lastSepIdx + 1, componentStr.length() - 3); |
There was a problem hiding this comment.
This is going to throw IndexOutOfBoundsException if the string doesn't contain anything after the separator, because the endIndex would be smaller than beginIndex param to substring().
This is unlikely to happen, but because this method reports unparseable strings as empty Optional then I think we should handle that case for consistency.
There was a problem hiding this comment.
This is going to throw
IndexOutOfBoundsExceptionif the string doesn't contain anything after the separator
It technically can't because this (private) method is only called from the tryParseFilename one which ensures the filename ended with .db.
With that said, the method was still a bit fragile around this as if the file ended by _.db, then the substring above didn't throw, but was returning an empty string, which was later parsed as an IndexComponentType, and this resulted in a "successful" parse but where the parsed IndexComponentType was null, which is unexpected and would have thrown. Anyway, I handled that case here and added tests for this.
Updates the file formatting methods for SAI `Version.AA` to handle the generation used by immutable components, enabling the option to use immutable components with version AA. This also add a new `cassandra.sai.immutable_components.min_version` system property that allows to defines the minimum verison starting from which the existing `cassandra.sai.immutable_components` option will apply. As of this patch, this defaults to `ca`, because it is the behavior before this patch, but it can change to `aa` to allow using immutable components with `Version.AA`. Fixes #15745.
pcmanus
force-pushed
the
CNDB-15745-immutable-components-in-AA
branch
from
70fb738 to
1484a69
Compare
|
✔️ Build ds-cassandra-pr-gate/PR-2079 approved by ButlerApproved by Butler |
…ion (#2079) Updates the file formatting methods for SAI `Version.AA` to handle the generation used by immutable components, enabling the option to use immutable components with version AA. This also add a new `cassandra.sai.immutable_components.min_version` system property that allows to defines the minimum verison starting from which the existing `cassandra.sai.immutable_components` option will apply. As of this patch, this defaults to `ca`, because it is the behavior before this patch, but it can change to `aa` to allow using immutable components with `Version.AA`. Fixes riptano/cndb#15745.
…ion (#2079) Updates the file formatting methods for SAI `Version.AA` to handle the generation used by immutable components, enabling the option to use immutable components with version AA. This also add a new `cassandra.sai.immutable_components.min_version` system property that allows to defines the minimum verison starting from which the existing `cassandra.sai.immutable_components` option will apply. As of this patch, this defaults to `ca`, because it is the behavior before this patch, but it can change to `aa` to allow using immutable components with `Version.AA`. Fixes riptano/cndb#15745.
Updates the file formatting methods for SAI
Version.AAto handle the generation used by immutable components, enabling the option to use immutable components with version AA.This also add a new
cassandra.sai.immutable_components.min_versionsystem property that allows to defines the minimum verison starting from which the existingcassandra.sai.immutable_componentsoption will apply. As of this patch, this defaults toca, because it is the behavior before this patch, but it can change toaato allow using immutable components withVersion.AA.Fixes https://github.com/riptano/cndb/issues/15745.