[
https://issues.apache.org/jira/browse/CASSANDRA-9462?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14611785#comment-14611785
]
Sylvain Lebresne commented on CASSANDRA-9462:
---------------------------------------------
I'm honestly not entirely sure I understand from all the comments above what
problems we have identified exactly and what exactly the branch to review aims
to fix, so I'm gonna lay out what I understand and what I would suggest we do
and you can comment on what I've missed, what I misunderstood and where you
disagree.
The first problem we have, the one that I think this ticket is about, is that
{{ViewTest.sstableInBounds}} is failing. My understanding of that failure is
that {{View.sstableInBounds}} pretends (through its signature) returning
sstables for any type of {{AbstractBounds}} (so with any permutation of
inclusive/exclusive start and end) but it transform a bound into an
{{Interval}} which is always inclusive so it always acts as if
{{AbstractBounds}} was actually a {{Bound}}.
Now, is that an actual problem for the code? I don't think it is for 2 reasons:
# the use of the interval tree is an optimization in the first place and having
a sstable returned that has no actual data for our range isn't a problem. And
given how incredibly unlikely it is in practice that a sstable is returned
unnecessarily due an exclusive bound being treated as inclusive, it's not even
a concrete performance issue.
# it's more anecdotal but at least some of the bounds passed to
{{sstableInBounds}} comes from a call to (the now misnamed)
{{Range.makeRowRange}}, which uses {{Token.maxKeyBound()}} which returns a
{{PartitionPosition}} that cannot be any real {{DecoratedKey}} (it's by design
bigger than any key having the original token). It follows that it doesn't
matter what type of {{AbstractBounds}} is returned by {{Range.makeRowRange}},
it will always select the same keys. Therefore passing its result to
{{sstableInBounds}} ends up being fine.
A second problem, which if I understand correctly is the "most serious" one
[~benedict] is referring to, is that there is a number of places where we pass
an {{AbstractBounds}} and the code silently assumes it is not wrapped (please
correct me if I misunderstood that). And it would indeed do the wrong thing if
we mistakenly passed a wrapping range. It's hard to say for sure that we never
make that mistake since it's hard to exhaustively list all the places where we
make those silent assertions, but at first glance I think we're good because:
* For reads, we unwrap stuff pretty early (in
{{StorageProxy.getRestrictedRanges}}).
* For streaming, we also clearly unwraps stuff in
{{StreamSession.addTransferRanges}}.
So sum up, my initial take away is that:
# The test failure does not expose a true bug in the code (at least not one
with user-visible consequence).
# The code make silent assertions regarding {{AbstractBounds}} in a number of
places making it easy to mess up, even though to the best of my knowledge there
is no evidence that we do mess up.
# The underlying cause of this is that the {{AbstractBounds}} API is confusing,
messy and makes mistake easy.
So, as I've said before, I'm all for rethinking the {{AbstractBounds}} API, but
that's not a minor undertaking and a different ticket. I've actually opened
CASSANDRA-9711 since I don't think we had one?.
Now in the short term, what I would suggest for this ticket is to add concrete
assertions in the place we've identified where there is silent assertions. I've
pushed a suggestion for this
[here|https://github.com/pcmanus/cassandra/commits/9462]. It's obviously a
band-aid, but it's simple enough that I think we can commit this in 2.1+ (it's
mostly stating assertions more clearly). That patch will force us to modify the
failing test so it only test inclusive bounds, thus fixing it, but my patch is
against 2.1 so the test changes are not included).
Now, there seems to be a third problem pointed by [~aweisberg] regarding some
of the behaviors of {{AbstractBounds}} methods: {{isWrapAround}} and
{{isEmpty}} more specifically.
Regarding {{Range.isWrapAround}}, it's definitively true that the existing
semantic is a weird and inconsistent regarding the min value. That said, and
unless we can identify actual bugs due to this semantic, I would prefer leaving
to CASSANDRA-9711 the task of fixing it since changing it is a lot of risk for
little and short-lived gains if we agree that we should do CASSANDRA-9711
anyway.
For {{isEmpty}}, to have {{isEmpty(bound(1, false), bound(1, true)) == true}}
actually feels right to me (alternatively, we could make isEmpty bitch because
that range is nonsensical). The behaviors with {{MIN}} are more obviously
broken, though the method is only called by {{SSTableScanner}} that, I think,
never pass it {{MIN}}. So one option could be to simply assert that in the
method (again, knowing that we'll change all this with CASSANDRA-9711). That
said, if you prefer fixing the method to work with {{MIN}}, I don't mind, but
let's open a separate ticket focused on that alone since as far as I can tell
it's not related to the {{ViewTest}} failure in any way.
> ViewTest.sstableInBounds is failing
> -----------------------------------
>
> Key: CASSANDRA-9462
> URL: https://issues.apache.org/jira/browse/CASSANDRA-9462
> Project: Cassandra
> Issue Type: Bug
> Components: Core
> Reporter: Benedict
> Assignee: Ariel Weisberg
> Fix For: 3.x, 2.1.x, 2.2.x
>
>
> CASSANDRA-8568 introduced new tests to cover what was DataTracker
> functionality in 2.1, and is now covered by the lifecycle package. This
> particular test indicates this method does not fulfil the expected contract,
> namely that more sstables are returned than should be.
> However while looking into it I noticed it also likely has a bug (which I
> have not updated the test to cover) wherein a wrapped range will only yield
> the portion at the end of the token range, not the beginning. It looks like
> we may have call sites using this function that do not realise this, so it
> could be a serious bug, especially for repair.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
