CASSANALYTICS-128: Add flag to allow bulk write to indexed tables#181
CASSANALYTICS-128: Add flag to allow bulk write to indexed tables#181jmckenzie-dev wants to merge 4 commits intoapache:trunkfrom
Conversation
Patch by Josh McKenzie; reviewed by YYYY for CASSANALYTICS-128
sarankk
left a comment
sarankk
left a comment
There was a problem hiding this comment.
Thanks Josh, Lgtm. 1 minor nit. Would it be useful to test writer with this flag in integration test too?
| public final int commitThreadsPerInstance; | ||
| public final double importCoordinatorTimeoutMultiplier; | ||
| public boolean quoteIdentifiers; | ||
| public boolean skipSecondaryIndexCheck; |
There was a problem hiding this comment.
| public boolean skipSecondaryIndexCheck; | |
| public final boolean skipSecondaryIndexCheck; |
| * with the understanding that the secondary indexes will NOT be updated by the bulk write and must be | ||
| * rebuilt separately after the job completes. | ||
| */ | ||
| SKIP_SECONDARY_INDEX_CHECK, |
There was a problem hiding this comment.
How about defining it in the spark conf, instead of write options?
The rationale is that the toggle is to for advanced use case and not directly related to the write behavior. There is another existing spark conf, org.apache.cassandra.spark.bulkwriter.BulkSparkConf#SKIP_CLEAN that skips cleaning up SSTable when job fails.
Admittedly, there are some existing inconsistency of where to have conf and where to have write options in the project.
There was a problem hiding this comment.
Would putting this in BulkSparkConf force it session-wide? i.e. a user would lose the ability to reason about and easily configure this setting on a per-table / per operation basis vs. the public exposure of it via WriterOptions?
My intuition right now is that this is something that probably should have been enabled by default all this time w/a configurable guardrail to turn it off, so while I'm sympathetic to the idea of taking a small step from "don't allow it" to "allow it but make it hard to use", the risk of this being easily accessible for users is that they'll bulk write to a table that will then have a long running index building operation happen in the background. Which, other than "load on node" and "application might read a partial index if you don't have automation that clearly delineates when a bulk insert and reindex finish from application accessing it", doesn't represent a structural or correctness risk to the data.
Eh, I didn't because ultimately it's a validation error inside the The rest of the integration path would effectively be testing whether bulk import on cassandra with a table that has legacy SecondaryIndexes on it works correctly. Which, to be completely honest, I'm not super confident of (dug around a bit and So maybe a follow up JIRA to add test coverage in C* proper for legacy 2i on imports would be reasonable? |
| this.ttl = MapUtils.getOrDefault(options, WriterOptions.TTL.name(), null); | ||
| this.timestamp = MapUtils.getOrDefault(options, WriterOptions.TIMESTAMP.name(), null); | ||
| this.quoteIdentifiers = MapUtils.getBoolean(options, WriterOptions.QUOTE_IDENTIFIERS.name(), false, "quote identifiers"); | ||
| this.skipSecondaryIndexCheck = MapUtils.getBoolean(options, WriterOptions.SKIP_SECONDARY_INDEX_CHECK.name(), false, "skip secondary index check"); |
There was a problem hiding this comment.
Should the default value be true?
| // process today. 2i and SAI have different ergonomics here regarding if stale data is served during index build; | ||
| // ultimately we want the bulk writer to also write native SAI index files alongside sstables but until | ||
| // then, this is allowable and fine for users who Know What They're Doing. | ||
| if (!skipSecondaryIndexCheck) |
There was a problem hiding this comment.
Can you please add a test if possible that check this behavior, basically the if block
4 participants
Patch by Josh McKenzie; reviewed by TBD for CASSANALYTICS-128