[GitHub] [cassandra] maedhroz commented on a diff in pull request #2267: CASSANDRA-18398: Trie-indexed SSTable format

["maedhroz (via GitHub)" <gi...@apache.org>]

maedhroz commented on code in PR #2267:
URL: https://github.com/apache/cassandra/pull/2267#discussion_r1162025086


##########
conf/cassandra.yaml:
##########
@@ -1924,8 +1924,14 @@ drop_compact_storage_enabled: false
 # which is used in some serialization to denote the format type in a compact way (such as local key cache); and 'name'
 # which will be used to recognize the format type - in particular that name will be used in sstable file names and in
 # stream headers so the name has to be the same for the same format across all the nodes in the cluster.
+# The first entry in this list is the format that will be used for newly-created SSTables. The other formats given
+# will be used to read any SSTables present in the data directories or streamed.
 sstable_formats:
   - class_name: org.apache.cassandra.io.sstable.format.big.BigFormat
     parameters:
       id: 0
       name: big
+  - class_name: org.apache.cassandra.io.sstable.format.bti.BtiFormat
+    parameters:
+      id: 1
+      name: bti

Review Comment:
   I wasn't able to look at the original SSTable format API configuration work, but there are a few things that worry me a bit:
   
   1.) Hard to know without actually writing the code, but it feels like some things would have been a bit simpler if we avoided `ParameterizedClass` and just used a `EncryptionOptions`-eqsue configuration object to contain the structure and validation logic for the formats.
   
   2.) There are a couple things here that feel very dangerous to expose to an operator via local configuration, with both local and cluster-wide implications.
   
   The first is having the `id` concept specified in config. If we start a node, write some SSTables, bring the node down, change the id of the primary format, then start the node again, I think we can break things like `KeyCacheSerializer#deserialize()`, which have expectations around previously written `id`s and how they map to formats. The ID concept feels like it should be statically and globally defined by the format itself. If you create a custom format, it should simply avoid conflicting w/ built-in `id`s/`name`s. (This may become a distributed problem around streaming too through a local configuration change on another node?)
   
   Second, while specifying a primary format (to write new SSTables) is necessary, allowing that to be determined by physical order in the YAML should IMO be avoided. It isn't a catastrophic risk, but one where you could silently write the wrong format for an accidental ordering mistake. The selection of a primary format should be explicit/not be a mystery in the absence of inline comments.
   
   Last (and I feel like we had this discussion around the Memtable API a while back), having to specify anything about the **built-in** formats in YAML space feels unnecessary. We'd still need something like this to specify custom formats, etc.
   
   I'm not pushing for a [Lucene-style SPI approach](https://www.elastic.co/blog/what-is-an-apache-lucene-codec) or anything in particular, but we need to at least discuss the 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