(improvement) remove query to TRIGGERS (which do not exist in ScyllaDB) #629
Conversation
With CoPilot, I removed the query, since this functionality doesn't exist in ScyllaDB at all. Refs: scylladb#453 Refs: https://scylladb.atlassian.net/browse/CUSTOMER-62 Signed-off-by: Yaniv Kaul <[email protected]>
|
CI failures do not seem related to this patch :-/ |
Lorak-mmk
left a comment
Lorak-mmk
left a comment
There was a problem hiding this comment.
Can you try to run integration tests with Cassandra and see if there are new failures caused by this change?
We do not currently do this in CI, there was work to do this but it was not finished: #339
Let's not make things more difficult for someone picking it up again.
Did you consider using those queries only with Cassandra, instead of removing them completely?
| indexes_result = self._handle_results(indexes_sucess, indexes_result, query_msg=indexes_query) | ||
| triggers_result = self._handle_results(triggers_success, triggers_result, query_msg=triggers_query) | ||
| return self._build_table_metadata(table_result[0], col_result, triggers_result, indexes_result) | ||
| return self._build_table_metadata(table_result[0], col_result, None, indexes_result) |
There was a problem hiding this comment.
Instead of passing None can you remove the argument from _build_table_metadata? Let's not introduce dead code.
| table_name = row[self._table_name_col] | ||
|
|
||
| col_rows = col_rows or self.keyspace_table_col_rows[keyspace_name][table_name] | ||
| trigger_rows = trigger_rows or self.keyspace_table_trigger_rows[keyspace_name][table_name] |
There was a problem hiding this comment.
Did you remove self.keyspace_table_trigger_rows as well?
| for trigger_row in trigger_rows: | ||
| trigger_meta = self._build_trigger_metadata(table_meta, trigger_row) | ||
| table_meta.triggers[trigger_meta.name] = trigger_meta | ||
|
|
This is a critical decision point we need to make - how much do we wish to remain compatible with Cassandra, and where. This was an edge case (who uses triggers, and what is the use case for it in the driver, etc.), so I decided to just drop it. I could have made it a Scylla only - unsure if I know at all code paths that we are running against Scylla, but I can try. In this specific case, I did not think it was needed, but I'm open to change that. |
Agreed. This is something that has come up in the past, and will come up in the future.
So this is a decision that must involve Core (@nyh or someone else knowledgeable about tests) and someone who knows Scylla development directions (you). Let's make the decision, and then continue with this PR. |
In an ideal world, we could have produced a driver that is just better than Datastax's, and a superset of it, so that everybody would just get our driver's instead of Datastax. But we don't live in a perfect world. Distros like Fedora have already dropped the Cassandra packages, so any hope that I had a decade ago that we can convince them to use our driver instead of Datastax's went out the window. The main reason why I'd still like our driver to work on Cassandra is that I want to allow a user who installed Scylla's driver to be able to run our tests (like test/cqlpy) against Cassandra. That being said, we don't need our driver to support features we never want to implement (like triggers) or specific protocol version which we don't want to test in our tests.
Exactly. I want to be able to use the Scylla driver to run tests against Cassandra. This means for example that if 10 years from now Cassandra only supports protocol version 7 and Scylla only supports protocol version 6, we need our drivers to allow both, and not just version 6. But our driver can still drop features we never intend to test (like triggers) or old protocol versions like (in the above example) version 5, which we won't use i neither Cassandra nor Scylla.
I think this is the main question, and it's @mykaul (for lack of a product person for Scylla core) decision. Did we ever officially decide that TRIGGERS will never ever be supported? If we decided that - we can remove it from the driver and also mention this decision in scylladb/scylladb#2205 (and perhaps even close it). But if we didn't decide this, why remove it from the driver? Just to make it harder to add it back in if we decide to implement this feature? |
3 participants
With CoPilot, I removed the query, since this functionality doesn't exist in ScyllaDB at all.
Refs: #453
Refs: https://scylladb.atlassian.net/browse/CUSTOMER-62
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.