[
https://issues.apache.org/jira/browse/CASSANDRA-16613?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17334702#comment-17334702
]
Sam Tunnicliffe edited comment on CASSANDRA-16613 at 4/28/21, 12:36 PM:
------------------------------------------------------------------------
Hi [~e.dimitrova] , thanks for picking this up, I should really have caught all
of these uses in either CASSANDRA-15299 or CASSANDRA-14973. It all looks
generally good to me, I just have a few minor comments/suggestions:
h3. {{DriverBurnTest}} et al
I wonder if we should a version param to
{{BurnTestUtil::generateQueryMessage}}, rather than hardcoding it. When called
from {{SimpleClient(Perf|Burn)Test}} it can be derived from
{{SimpleClient.connection.getVersion()}}. {{DriverBurnTest}} is a bit more of a
pain, but there I think we'd need to add a parameter to {{perfTest}} and pass
the version for each test explicitly.
h3. {{QueryPagerTest}}
In the tests you already changed, could we loop over
{{ProtocolVersion.SUPPORTED}} rather than specifying the versions directly?
h3. {{ClientWarningsTest}}
For this (and I think for {{ViewFilteringTest}} & {{ViewLongTest}}) we could
make them {{@RunWith(Parameterized.class)}} to exercise all the relevant
versions. I did this in {{ClientRequestSizeMetricsTest}}, but I've overlooked
these other version-dependent tests sorry.
h3. {{PagingStateTest}}
The new test LGTM and also sufficient. The paging state format went through
some (undocumented) changes between V3 and V4, hence the need for the mixed
version tests. There's been no such change for V5 so we should be good here.
was (Author: beobal):
Hi @Ekaterina, thanks for picking this up, I should really have caught all of
these uses in either CASSANDRA-15299 or CASSANDRA-14973. It all looks generally
good to me, I just have a few minor comments/suggestions:
h3. {{DriverBurnTest}} et al
I wonder if we should a version param to
{{BurnTestUtil::generateQueryMessage}}, rather than hardcoding it. When called
from {{SimpleClient(Perf|Burn)Test}} it can be derived from
{{SimpleClient.connection.getVersion()}}. {{DriverBurnTest}} is a bit more of a
pain, but there I think we'd need to add a parameter to {{perfTest}} and pass
the version for each test explicitly.
h3. {{QueryPagerTest}}
In the tests you already changed, could we loop over
{{ProtocolVersion.SUPPORTED}} rather than specifying the versions directly?
h3. {{ClientWarningsTest}}
For this (and I think for {{ViewFilteringTest}} & {{ViewLongTest}}) we could
make them {{@RunWith(Parameterized.class)}} to exercise all the relevant
versions. I did this in {{ClientRequestSizeMetricsTest}}, but I've overlooked
these other version-dependent tests sorry.
h3. {{PagingStateTest}}
The new test LGTM and also sufficient. The paging state format went through
some (undocumented) changes between V3 and V4, hence the need for the mixed
version tests. There's been no such change for V5 so we should be good here.
> ProtocolVersion.V4 is still used in places in the code
> ------------------------------------------------------
>
> Key: CASSANDRA-16613
> URL: https://issues.apache.org/jira/browse/CASSANDRA-16613
> Project: Cassandra
> Issue Type: Bug
> Reporter: Ekaterina Dimitrova
> Assignee: Ekaterina Dimitrova
> Priority: Normal
> Fix For: 4.0-rc
>
>
> While working on CASSANDRA-16567, [~adelapena] observed that
> _ProtocolVersion.V4_ is used in _ViewTest_.
> I decided to do a quick grep and observed a list of places where we still
> refer to V4 and it seems at least in many of the tests that was left not
> intentionally.
> This ticket is to verify the usage of _ProtocolVersion.V4_ in the codebase
> and bump it to V5 or default version, similar to what was done in
> CASSANDRA-16567, wherever there is a need.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
