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/04/23 12:33:03 UTC
[GitHub] [cassandra] bereng opened a new pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
bereng opened a new pull request #559:
URL: https://github.com/apache/cassandra/pull/559
----------------------------------------------------------------
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] bereng closed pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng closed pull request #559:
URL: https://github.com/apache/cassandra/pull/559
----------------------------------------------------------------
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] blerer commented on a change in pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #559:
URL: https://github.com/apache/cassandra/pull/559#discussion_r426489932
##########
File path: src/java/org/apache/cassandra/index/internal/composites/CompositesSearcher.java
##########
@@ -231,16 +231,27 @@ private IndexEntry findEntry(Clustering clustering)
while (entriesIdx < entries.size())
{
IndexEntry entry = entries.get(entriesIdx++);
+ Clustering indexedEntryClustering = entry.indexedEntryClustering;
// The entries are in clustering order. So that the requested entry should be the
// next entry, the one at 'entriesIdx'. However, we can have stale entries, entries
// that have no corresponding row in the base table typically because of a range
// tombstone or partition level deletion. Delete such stale entries.
- int cmp = comparator.compare(entry.indexedEntryClustering, clustering);
+ int cmp = comparator.compare(indexedEntryClustering, clustering);
assert cmp <= 0; // this would means entries are not in clustering order, which shouldn't happen
if (cmp == 0)
return entry;
else
- staleEntries.add(entry);
+ {
+ boolean isStaticClustering = dataIter.metadata().hasStaticColumns();
Review comment:
It is effectively a bit hard to read. I would have done something like:
```
boolean hasStaticColumns = dataIter.metadata().hasStaticColumns();
// COMPACT COMPOSITE tables support null values in there clustering key but
// those tables do not support static columns. By consequence if a table
// has some static columns and all its clustering key elements are null
// it means that the partition exists and contains only static data
if (!hasStaticColumns() || !containsOnlyNullValues(indexedEntryClustering)
staleEntries.add(entry);
```
What do you think?
----------------------------------------------------------------
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] bereng commented on a change in pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #559:
URL: https://github.com/apache/cassandra/pull/559#discussion_r414331879
##########
File path: src/java/org/apache/cassandra/index/internal/composites/CompositesSearcher.java
##########
@@ -231,16 +231,27 @@ private IndexEntry findEntry(Clustering clustering)
while (entriesIdx < entries.size())
{
IndexEntry entry = entries.get(entriesIdx++);
+ Clustering indexedEntryClustering = entry.indexedEntryClustering;
// The entries are in clustering order. So that the requested entry should be the
// next entry, the one at 'entriesIdx'. However, we can have stale entries, entries
// that have no corresponding row in the base table typically because of a range
// tombstone or partition level deletion. Delete such stale entries.
- int cmp = comparator.compare(entry.indexedEntryClustering, clustering);
+ int cmp = comparator.compare(indexedEntryClustering, clustering);
assert cmp <= 0; // this would means entries are not in clustering order, which shouldn't happen
if (cmp == 0)
return entry;
else
- staleEntries.add(entry);
+ {
+ boolean isStaticClustering = dataIter.metadata().hasStaticColumns();
Review comment:
Might read a bit awkward, but given I am in a hot path I wanted to incur the cost to test for nulls only _if_ needed
----------------------------------------------------------------
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] bereng commented on a change in pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #559:
URL: https://github.com/apache/cassandra/pull/559#discussion_r427101980
##########
File path: src/java/org/apache/cassandra/index/internal/composites/CompositesSearcher.java
##########
@@ -231,16 +231,27 @@ private IndexEntry findEntry(Clustering clustering)
while (entriesIdx < entries.size())
{
IndexEntry entry = entries.get(entriesIdx++);
+ Clustering indexedEntryClustering = entry.indexedEntryClustering;
// The entries are in clustering order. So that the requested entry should be the
// next entry, the one at 'entriesIdx'. However, we can have stale entries, entries
// that have no corresponding row in the base table typically because of a range
// tombstone or partition level deletion. Delete such stale entries.
- int cmp = comparator.compare(entry.indexedEntryClustering, clustering);
+ int cmp = comparator.compare(indexedEntryClustering, clustering);
assert cmp <= 0; // this would means entries are not in clustering order, which shouldn't happen
if (cmp == 0)
return entry;
else
- staleEntries.add(entry);
+ {
+ boolean isStaticClustering = dataIter.metadata().hasStaticColumns();
Review comment:
@blerer if you give me thumbs up I'll rebase, squash, forward merge and run CI before handing it over to you for committing.
----------------------------------------------------------------
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] blerer commented on a change in pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
blerer commented on a change in pull request #559:
URL: https://github.com/apache/cassandra/pull/559#discussion_r426489932
##########
File path: src/java/org/apache/cassandra/index/internal/composites/CompositesSearcher.java
##########
@@ -231,16 +231,27 @@ private IndexEntry findEntry(Clustering clustering)
while (entriesIdx < entries.size())
{
IndexEntry entry = entries.get(entriesIdx++);
+ Clustering indexedEntryClustering = entry.indexedEntryClustering;
// The entries are in clustering order. So that the requested entry should be the
// next entry, the one at 'entriesIdx'. However, we can have stale entries, entries
// that have no corresponding row in the base table typically because of a range
// tombstone or partition level deletion. Delete such stale entries.
- int cmp = comparator.compare(entry.indexedEntryClustering, clustering);
+ int cmp = comparator.compare(indexedEntryClustering, clustering);
assert cmp <= 0; // this would means entries are not in clustering order, which shouldn't happen
if (cmp == 0)
return entry;
else
- staleEntries.add(entry);
+ {
+ boolean isStaticClustering = dataIter.metadata().hasStaticColumns();
Review comment:
It is effectively a bit hard to read. I would have done something like:
boolean hasStaticColumns = dataIter.metadata().hasStaticColumns();
```
// COMPACT COMPOSITE tables support null values in there clustering key but
// those tables do not support static columns. By consequence if a table
// has some static columns and all its clustering key elements are null
// it means that the partition exists and contains only static data
if (!hasStaticColumns() || !containsOnlyNullValues(indexedEntryClustering)
staleEntries.add(entry);
```
What do you think?
----------------------------------------------------------------
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] bereng commented on a change in pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng commented on a change in pull request #559:
URL: https://github.com/apache/cassandra/pull/559#discussion_r427101980
##########
File path: src/java/org/apache/cassandra/index/internal/composites/CompositesSearcher.java
##########
@@ -231,16 +231,27 @@ private IndexEntry findEntry(Clustering clustering)
while (entriesIdx < entries.size())
{
IndexEntry entry = entries.get(entriesIdx++);
+ Clustering indexedEntryClustering = entry.indexedEntryClustering;
// The entries are in clustering order. So that the requested entry should be the
// next entry, the one at 'entriesIdx'. However, we can have stale entries, entries
// that have no corresponding row in the base table typically because of a range
// tombstone or partition level deletion. Delete such stale entries.
- int cmp = comparator.compare(entry.indexedEntryClustering, clustering);
+ int cmp = comparator.compare(indexedEntryClustering, clustering);
assert cmp <= 0; // this would means entries are not in clustering order, which shouldn't happen
if (cmp == 0)
return entry;
else
- staleEntries.add(entry);
+ {
+ boolean isStaticClustering = dataIter.metadata().hasStaticColumns();
Review comment:
@blerer if you give me thumbs up on the latest commit I just did I'll rebase, squash, forward merge and run CI before handing it over to you for committing.
----------------------------------------------------------------
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] bereng commented on pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #559:
URL: https://github.com/apache/cassandra/pull/559#issuecomment-618830473
[CI run](https://circleci.com/workflow-run/bf02e9ad-a1e4-4700-bfba-daed55fcfc3c) passes utests which are the main ones to worry about. Failing dtests are due to connection issues, I tested some locally and they pass
----------------------------------------------------------------
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] bereng commented on pull request #559: CASSANDRA-13666 Avoid rendering static 2i entries as stales and removing them
Posted by GitBox <gi...@apache.org>.
bereng commented on pull request #559:
URL: https://github.com/apache/cassandra/pull/559#issuecomment-630841680
@blerer squahsed, rebased & forward merged. Thx!
----------------------------------------------------------------
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