[jira] [Comment Edited] (CASSANDRA-12792) delete with timestamp long.MAX_VALUE for the whole key creates tombstone that cannot be removed.
[
https://issues.apache.org/jira/browse/CASSANDRA-12792?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15672811#comment-15672811
]
Joel Knighton edited comment on CASSANDRA-12792 at 11/17/16 5:41 AM:
-
I've pushed rebased, updated branches and ran CI. CI looks clean relative to
upstream. I made your proposed fixes regarding hasTimestamp, null checking, and
lambda usage in the 3.0+ branches. While looking at the code, I realized that
the {{PurgeEvaluator}} interface was no longer necessary after an earlier
refactor and that comparable internals seem to use {{Predicate}} directly. I
adopted this approach in 2.2+ and changed the 2.2 branch to use anonymous
classes, since I thought this made it a little easier to follow. Let me know
your thoughts on these additional changes.
||branch||testall||dtest||
|[CASSANDRA-12792-2.2|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-2.2]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-2.2-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-2.2-dtest]|
|[CASSANDRA-12792-3.0|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-3.0]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.0-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.0-dtest]|
|[CASSANDRA-12792-3.X|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-3.X]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.X-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.X-dtest]|
|[CASSANDRA-12792-trunk|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-trunk]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-trunk-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-trunk-dtest]|
was (Author: jkni):
I've pushed rebased, updated branches and ran CI. CI looks clean relative to
upstream. I made your proposed fixes regarding hasTimestamp, null checking, and
lambda usage in the 3.0+ branches. While looking at the code, I realized that
the {{PurgeEvaluator}} interface was no longer necessary after an earier
refactor and that comparable internals seem to use {{Predicate}} directly. I
adopted this approach in 2.2+ and changed the 2.2 branch to use anonymous
classes, since I thought this made it a little easier to follow. Let me know
your thoughts on these additional changes.
||branch||testall||dtest||
|[CASSANDRA-12792-2.2|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-2.2]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-2.2-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-2.2-dtest]|
|[CASSANDRA-12792-3.0|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-3.0]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.0-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.0-dtest]|
|[CASSANDRA-12792-3.X|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-3.X]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.X-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-3.X-dtest]|
|[CASSANDRA-12792-trunk|https://github.com/jkni/cassandra/tree/CASSANDRA-12792-trunk]|[testall|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-trunk-testall]|[dtest|http://cassci.datastax.com/view/Dev/view/jkni/job/jkni-CASSANDRA-12792-trunk-dtest]|
> delete with timestamp long.MAX_VALUE for the whole key creates tombstone that
> cannot be removed.
> -
>
> Key: CASSANDRA-12792
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12792
> Project: Cassandra
> Issue Type: Bug
> Components: Compaction
>Reporter: Ian Ilsley
>Assignee: Joel Knighton
>
> In db/compaction/LazilyCompactedRow.java
> we only check for < MaxPurgeableTimeStamp
> eg:
> (this.maxRowTombstone.markedForDeleteAt < getMaxPurgeableTimestamp())
> this should probably be <=
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Comment Edited] (CASSANDRA-12792) delete with timestamp long.MAX_VALUE for the whole key creates tombstone that cannot be removed.
[
https://issues.apache.org/jira/browse/CASSANDRA-12792?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15636900#comment-15636900
]
Branimir Lambov edited comment on CASSANDRA-12792 at 11/4/16 4:29 PM:
--
I meant that you need the same logic for sstables as well: currently if you
have sstables and they all don't have the row, you will go into the 'else' with
{{Long.MAX_VALUE}} instead of always true.
was (Author: blambov):
You need the same logic for sstables as well: If you have sstables and they all
don't have the row, you will go into the 'else' with {{Long.MAX_VALUE}} instead
of always true.
> delete with timestamp long.MAX_VALUE for the whole key creates tombstone that
> cannot be removed.
> -
>
> Key: CASSANDRA-12792
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12792
> Project: Cassandra
> Issue Type: Bug
> Components: Compaction
>Reporter: Ian Ilsley
>Assignee: Joel Knighton
>
> In db/compaction/LazilyCompactedRow.java
> we only check for < MaxPurgeableTimeStamp
> eg:
> (this.maxRowTombstone.markedForDeleteAt < getMaxPurgeableTimestamp())
> this should probably be <=
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Comment Edited] (CASSANDRA-12792) delete with timestamp long.MAX_VALUE for the whole key creates tombstone that cannot be removed.
[
https://issues.apache.org/jira/browse/CASSANDRA-12792?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15636859#comment-15636859
]
Joel Knighton edited comment on CASSANDRA-12792 at 11/4/16 4:15 PM:
Good observation on the {{hasPurgeEvaluator}} flag. I'll remove that in my
follow up patch. I'll also try the lambda approach on 3.0+; I think you're
right in valuing clarity on the version going forward here. When doing that,
I'll look for chances to reduce verbosity. It does feel bigger than it needs to
be.
I'm not sure I understand the {{hasMemtableCf}}/{{hasTimestamp}} distinction.
The only place that {{hasMemtableCf}} is used is dictating whether we return
the {{AlwaysTruePurgeEvaluator}} or a {{TimestampedPurgeEvaluator}}. In the
case that the value of {{hasMemtableCF}} matters, we already know that
{{filteredSSTables}} is empty. This means that the current {{minTimestamp}}
when we start inspecting memtables is {{Long.MAX_VALUE}}. We would need to set
{{hasTimestamp}} whenever the partition's mintimestamp is less than or equal to
this {{minTimestamp}}, since we need a {{TimestampedPurgeEvaluator}} in the
case where a value in the partition is timestamped with {{Long.MAX_VALUE}}.
Since any long value will be less than or equal to {{Long.MAX_VALUE}}, it seems
like setting a flag whenever we take the minTimestamp from a partition in a
memtable reduces to setting a flag whenever we have a partition in a memtable.
I'm probably misunderstanding something here.
was (Author: jkni):
Good observation on the {{hasPurgeEvaluator}} flag. I'll remove that in my
follow up patch. I'll also try the lambda approach on 3.0+; I think you're
right in valuing clarity on the version going forward here.
I'm not sure I understand the {{hasMemtableCf}}/{{hasTimestamp}} distinction.
The only place that {{hasMemtableCf}} is used is dictating whether we return
the {{AlwaysTruePurgeEvaluator}} or a {{TimestampedPurgeEvaluator}}. In the
case that the value of {{hasMemtableCF}} matters, we already know that
{{filteredSSTables}} is empty. This means that the current {{minTimestamp}}
when we start inspecting memtables is {{Long.MAX_VALUE}}. We would need to set
{{hasTimestamp}} whenever the partition's mintimestamp is less than or equal to
this {{minTimestamp}}, since we need a {{TimestampedPurgeEvaluator}} in the
case where a value in the partition is timestamped with {{Long.MAX_VALUE}}.
Since any long value will be less than or equal to {{Long.MAX_VALUE}}, it seems
like setting a flag whenever we take the minTimestamp from a partition in a
memtable reduces to setting a flag whenever we have a partition in a memtable.
I'm probably misunderstanding something here.
> delete with timestamp long.MAX_VALUE for the whole key creates tombstone that
> cannot be removed.
> -
>
> Key: CASSANDRA-12792
> URL: https://issues.apache.org/jira/browse/CASSANDRA-12792
> Project: Cassandra
> Issue Type: Bug
> Components: Compaction
>Reporter: Ian Ilsley
>Assignee: Joel Knighton
>
> In db/compaction/LazilyCompactedRow.java
> we only check for < MaxPurgeableTimeStamp
> eg:
> (this.maxRowTombstone.markedForDeleteAt < getMaxPurgeableTimestamp())
> this should probably be <=
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
