scylladb
(improvement) remove query to TRIGGERS (which do not exist in ScyllaDB)
With CoPilot, I removed the query, since this functionality doesn't exist in ScyllaDB at all.
Refs: https://github.com/scylladb/python-driver/issues/453 Refs: https://scylladb.atlassian.net/browse/CUSTOMER-62
Pre-review checklist
- [x] I have split my patch into logically separate commits.
- [ x All commit messages clearly explain what they change and why.
- [ ] I added relevant tests for new features and bug fixes.
- [x] All commits compile, pass static checks and pass test.
- [x] PR description sums up the changes and reasons why they should be introduced.
- [x] I have provided docstrings for the public items that I want to introduce.
- [ ] I have adjusted the documentation in
./docs/source/. - [x] I added appropriate
Fixes:annotations to PR description.
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?
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.
This is a critical decision point we need to make - how much do we wish to remain compatible with Cassandra, and where.
Agreed. This is something that has come up in the past, and will come up in the future. I personally don't feel like I'm in position to make an informed decision about this. I assume we don't really need to worry about external users that run the driver with Cassandra. What we need to worry about is:
- Driver's tests. That's why I asked you to check for new failures with Cassandra
- Internal users: scylla tests, dtests etc. I know there are some compatiblity tests sometimes executed with Cassandra, but I have no idea about specifics, and about whats required from the driver (cc @nyh ?)
- Future changes to Scylla: Scylla strives for C* compatiblity (right?), so I assume unimplemented C* features may get implemented in the future. If some features definitely won't, then it should be fine to remove them (but again, it's not my call).
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.
I assume we don't really need to worry about external users that run the driver with Cassandra.
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.
What we need to worry about is:
1. Driver's tests. That's why I asked you to check for new failures with Cassandra 2. Internal users: scylla tests, dtests etc. I know there are some compatiblity tests sometimes executed with Cassandra, but I have no idea about specifics, and about whats required from the driver (cc @nyh ?)
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.
3. Future changes to Scylla: Scylla strives for C* compatiblity (right?), so I assume unimplemented C* features may get implemented in the future. If some features definitely won't, then it should be fine to remove them (but again, it's not my call).
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 https://github.com/scylladb/scylladb/issues/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?
There is hardly any use case for the drivers for triggers, other than some metadata the app may be able to do something about. I'm comfortable with testing it against Cassandra and seeing that it does not break any functionality we actually dropped from the driver - and everything else just works.
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?
If we ever decide to implement triggers, the code is in Git. Until then, every new control connection of every client sends a useless query to the node. I've seen it in customer cases as one of those consuming a reader concurrency semaphore, and that is just silly.
Could you check if its difficult to only send those requests to Cassandra, and not to Scylla? If its feasible, maybe we can do this? If its too much work, let's remove this. In that case please try to remove all remnants.
Could you check if its difficult to only send those requests to Cassandra, and not to Scylla? If its feasible, maybe we can do this? If its too much work, let's remove this. In that case please try to remove all remnants.
It is definitely possible, based on connection we can figure out if scylla features is present and have a separate SchemaParser class which will be way better way to handle these problems.
Could you check if its difficult to only send those requests to Cassandra, and not to Scylla? If its feasible, maybe we can do this? If its too much work, let's remove this. In that case please try to remove all remnants.
It is definitely possible, based on connection we can figure out if scylla features is present and have a separate SchemaParser class which will be way better way to handle these problems.
Done, please review.
I think CI failed on flakiness - https://github.com/scylladb/python-driver/issues/603 ?
@calebxyz - this is a nit that is also relevant to our connection storm. Just a small example of course.