You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/12/07 21:13:34 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
smiklosovic opened a new pull request #844:
URL: https://github.com/apache/cassandra/pull/844
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic closed pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #844:
URL: https://github.com/apache/cassandra/pull/844
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] maedhroz commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538635567
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
I would like Zhao's proposal if this was around an optional feature, but in some ways RFP is *required*, given an index implementation can't be correct at CL > ONE/LO without supporting it. (i.e. In a perfect world we wouldn't need the method at all.) In terms of naming, I'm fine with `supportsFiltering()` as long as the JavaDoc outlines RFP as an example of why we would need to filter.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r539286644
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
Thanks for your in-depth answer, I would go for `true` too, hence there is nothing to change here. I am resolving this.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538765456
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
@adelapena @maedhroz is not it true that as of today, all indexes are supporting filtering protection but SASI does not? All implementations of a custom index do support filtering based on the current code as it is, only SASI does not so having this on "false" as default does not make sense to me. We would have to go over all impls of indexes and change it to true there or I am missing something. I would make the default what is used more frequently and it seems we are supporting filtering for all indexes but not sasi ones.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538702151
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
I dont agree with `supportsFiltering`, it is pretty non-telling ... like filtering of what? I would go with this name as is, without `IndexFeature` as argument. Deal?
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] jasonstack commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
jasonstack commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538605610
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
how about making it more generic? like `supports(IndexFeature feature)`. it will be useful for future indexes. WDYT?
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538766522
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
ehhhhhhh wait, I need to reconsider this.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] adelapena commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538556889
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
I'd move down the new method from the [`Update processing`](https://github.com/apache/cassandra/blob/bc9a554c29248ed7733608b995cc26f410e2ba78/src/java/org/apache/cassandra/index/Index.java#L405) section to the [`Querying`](https://github.com/apache/cassandra/blob/bc9a554c29248ed7733608b995cc26f410e2ba78/src/java/org/apache/cassandra/index/Index.java#L537) section, probably right below `validate(ReadCommand)`.
##########
File path: src/java/org/apache/cassandra/service/reads/DataResolver.java
##########
@@ -121,11 +121,15 @@ private boolean needsReplicaFilteringProtection()
if (command.rowFilter().isEmpty())
return false;
- IndexMetadata indexDef = command.indexMetadata();
- if (indexDef != null && indexDef.isCustom())
+ ColumnFamilyStore cfs = ColumnFamilyStore.getIfExists(command.metadata().keyspace, command.metadata().name);
Review comment:
I think that for better performance we should check whether the `IndexMetadata` is null or not-custom before opening `ColumnFamilyStore`. Also, we can open the store with the shorter `ColumnFamilyStore.getIfExists(command.metadata().id)`.
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
Not sure what should be the default argument, I think that for compatibility we should return `false`.
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
I wonder if it would make sense to use a more generic name for this method, something like `supportsFiltering`. @maedhroz wdyt?
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
We could pass the `RowFilter` as an argument, so implementations can change their response depending on the specific query. For example, SASI could use RFP if no analyzed columns are involved. Not saying that we should change SASI behaviour in this ticket, just having the argument in the interface.
##########
File path: src/java/org/apache/cassandra/index/sasi/SASIIndex.java
##########
@@ -249,6 +249,11 @@ public long getEstimatedResultRows()
public void validate(PartitionUpdate update) throws InvalidRequestException
{}
+ public boolean supportsReplicaFilteringProtection()
Review comment:
```suggestion
@Override
public boolean supportsReplicaFilteringProtection()
```
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] adelapena commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538607713
##########
File path: src/java/org/apache/cassandra/service/reads/DataResolver.java
##########
@@ -27,6 +27,7 @@
import org.apache.cassandra.config.DatabaseDescriptor;
import org.apache.cassandra.cql3.statements.schema.IndexTarget;
+import org.apache.cassandra.db.ColumnFamilyStore;
Review comment:
Nit: the import of `IndexTarget` right above is not used anymore.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] smiklosovic commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538765456
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
@adelapena @maedhroz is not it true that as of today, all indexes are supporting filtering protection but SASI does not? All implementations of a custom index do support filtering based on the current code as it is, only the impls of SASI ones are not so having this on "false" as default does not make sense to me. We would have to go over all impls of indexes and change it to true there or I am missing something. I would make the default what is used more frequently and it seems we are supporting filtering for all indexes but not sasi ones.
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] adelapena commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
adelapena commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r539250980
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
+ {
+ return true;
Review comment:
The only index implementation in the codebase other than SASI is regular indexes (`CassandraIndex` and its subclasses). Regular indexes aren't custom indexes so the new method won't ever be called for them. Instead RFP is always used with regular indexes. There are also some mock index implementations in unit tests but they are not used in a way that RFP is involved. So regarding indexes in Cassandra codebase the default value is only relevant for SASI, which is a minimal change. As for third party indexes out there, the question for the default value is not what value is more frequently used but what value is safer in case the implementations miss the changes in the index API and forget to implement the method. It's worth mentioning that previously existing implementations might be already compatible with SRP without any changes if they don't use custom syntax nor transformations of the indexed data. On the one hand, I'd say that indexes with wrongly enabled SRP are far worse
than indexes with wrongly disabled SRP, which leads me to prefer a default of `false` for the sake of compatibility. On the other hand, it's very desirable to have SRP enabled since it's a bug fix, and a wrongly enabled SRP is far easy to detect, which makes me prefer a default of `true` for the sake of correctness. wdyt?
----------------------------------------------------------------
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.
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
[GitHub] [cassandra] maedhroz commented on a change in pull request #844: CASSANDRA-16331 Extend the exclusion of replica filtering protection to other indices instead of just SASI
Posted by GitBox <gi...@apache.org>.
maedhroz commented on a change in pull request #844:
URL: https://github.com/apache/cassandra/pull/844#discussion_r538635567
##########
File path: src/java/org/apache/cassandra/index/Index.java
##########
@@ -426,6 +426,16 @@ public Indexer indexerFor(DecoratedKey key,
WriteContext ctx,
IndexTransaction.Type transactionType);
+ /**
+ * Tells whether this index supports replica fitering protection or not.
+ *
+ * @return true if this index supports replica filtering protection, false otherwise
+ */
+ default boolean supportsReplicaFilteringProtection()
Review comment:
I would like Zhao's proposal if this was around an optional feature, but in some ways RFP is *required*, given an index implementation can't be correct without supporting it. (i.e. In a perfect world we wouldn't need the method at all.) In terms of naming, I'm fine with `supportsFiltering()` as long as the JavaDoc outlines RFP as an example of why we would need to filter.
----------------------------------------------------------------
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.
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