Re: [DISCUSS] Client protocol changes (Was: 20200217 4.0 Status Update)

[Jorge Bay Gondra]

There were some discussions on Slack whether CASSANDRA-2848 should be in
scope for 4.0 or not, given that its an enhancement.

I'm +1 on descoping it and leave it for a later 4.x

As it involves a change in the protocol, we could use non-breaking changes
to add these types of features to protocol_v5 in 4.x without a protocol
version bump (new flag + new SUPPORTED option), like I've proposed above.

On Thu, Feb 20, 2020 at 2:52 PM Aleksey Yeshchenko
 wrote:

> For what it’s worth, we could trivially implement support for passing down
> the timeout in 4.0.0, so that both the server and the client are able to
> parse the frames with and without them, but delay implementation of the
> server side logic necessary for terminating requests early until a further
> minor (4.1/4.0.1).
>
> > On 19 Feb 2020, at 15:39, Jorge Bay Gondra 
> wrote:
> >
> > Also worth mentioning that, from the driver's perspective, it has to
> > support a protocol version during the lifetime of the C* version line.
> For
> > example, the drivers should drop support for protocol v3 after C* 2.1
> goes
> > EOL, somewhere this year, a protocol that was released back in 2014.
> >
> > We _could_ establish looser restrictions on whats a breaking change in a
> > protocol version (needing a version bump), that way the driver can
> support
> > a protocol version partially and a protocol version could evolve within
> > those limits.
> >
> > Back to the query timeout, a new query flag that can only be set by the
> > client is not a breaking change for the driver. The driver could ask
> > whether that feature of the protocol v5 is supported (OPTIONS/SUPPORTED
> > messages), without having to identify the server version.
> >
> > On Wed, Feb 19, 2020 at 12:24 AM Benedict Elliott Smith <
> bened...@apache.org>
> > wrote:
> >
> >> Behaviours don't have to be switched only with a new protocol version;
> >> it's possible to support optional feature/modifier flags, the support
> for
> >> which is negotiated with a client on connection.
> >>
> >> A protocol version change seems reasonable to limit to major releases,
> but
> >> a protocol feature seems perfectly reasonable to introduce in a minor, I
> >> think?  Ideally a version change would only be necessary for forced
> >> deprecation/standardisation of features, behaviour and stream encodings.
> >>
> >>
> >> On 18/02/2020, 21:53, "Jeff Jirsa"  wrote:
> >>
> >>A few notes:
> >>
> >>- Protocol changes add work to the rest of the ecosystem. Drivers
> have
> >> to
> >>update, etc.
> >>- Nobody expects protocol changes in minors, though it's less of a
> >> concern
> >>if we don't deprecate out the older version. E.g. if 4.0 launches
> with
> >>protocol v4 and protocol v5, and then 4.0.2 adds protocol v6, do we
> >>deprecate out v4? If yes, you potentially break clients that only
> >> supported
> >>v3 and v4 in a minor version upgrade, which is unexpected. If not,
> how
> >> many
> >>protocol versions are you willing to support at any given time?
> >>- Having protocol changes introduces risk. Paging behavior across
> >> protocol
> >>versions is the site of a number of different bugs recently.
> >>
> >>
> >>On Tue, Feb 18, 2020 at 1:46 PM Tolbert, Andrew  >
> >> wrote:
> >>
> >>> I don't know the technical answer, but I suspect two motivations for
> >>> doing new protocol versions in major releases could include:
> >>>
> >>> * protocol changes can be tied to feature changes that typically come
> >>> in a major release.
> >>> * protocol changes should be as infrequent as major releases.  Each
> >>> new protocol version is another thing in the test matrix that needs
> >> to
> >>> be tested.
> >>>
> >>> That last point can make it hard to get new changes in. If something
> >>> doesn't make the upcoming protocol version, it might be years before
> >>> another one, but I also think it's worth it to do this infrequently
> >> as
> >>> it makes maintaining client and server code easier if there are less
> >>> protocol versions to worry about.
> >>>
> >>> On the client-side, libraries themselves should be avoiding making
> >>> Cassandra version checks when detecting capabilities.  There are a
> >> few
> >>> exceptions, such as system table parsing for schema & peers,
> >>> but those aren't related to the protocol.
> >>>
> >>> Thanks,
> >>> Andy
> >>>
> >>>
> >>>
> >>>
> >>>
> >>> On Tue, Feb 18, 2020 at 1:22 PM Nate McCall 
> >> wrote:
> 
>  [Moving to new message thread]
> 
>  Thanks for bringing this up, Jordan.
> 
>  IIRC, this was more a convention than a technical reason. Though I
> >> could
> >>> be
>  completely misremembering this.
> 
>  -- Forwarded message -
>  From: Jordan West 
>  Date: Wed, Feb 19, 2020 at 10:13 AM
>  Subject: Re: 20200217 4.0 Status Update
>  To: 
> 
> 
>  On Mon, Feb 17, 2020 at 12:52 PM Jeff Jirsa 
> >> wrote:
> 
> >
> > beyond the client proto change

Re: [proposal] Introduce AssertJ in test framework

[Kevin Gallardo]

Hi,

Once you get to know assertJ, it's impossible to go back to basic
> assertions of JUnit


Definitely with you on that :)

Great to see people are excited about this, thanks for the responses. Given
the positive feedback I have created CASSANDRA-15631
 as a first step.

My proposal to try to get this going smoothly would be:

1. introduce the dependency and rewrite an existing test suite to showcase
it as a reference
2. encourage new tests to be written with AssertJ once the dep is added
3. add a mention of the preferred assertion library in the contributing
guidelines and link to the reference for an example
(4. potentially migrating other existing tests to AssertJ in the long term)

wdyt?


On Tue, Mar 10, 2020 at 6:24 PM Joshua McKenzie 
wrote:

> Strong +1 here. Many of you know I'm a C# / LINQ junkie though. ;)
>
> On Tue, Mar 10, 2020 at 3:55 PM DuyHai Doan  wrote:
>
> > Definitely +1
> >
> > Coding in Java every day, I can't write test without assertJ. Once you
> get
> > to know assertJ, it's impossible to go back to basic assertions of JUnit
> > that feels really boilerplate
> >
> >
> >
> > On Tue, Mar 10, 2020 at 8:53 PM Jordan West  wrote:
> >
> > > If it encourages more  and higher quality test writing +1 (nb). Also,
> low
> > > risk given it’s a test dep.
> > >
> > > Using QuickTheories as an example, merging it with a new or updated
> test
> > > could be a good way to get it merged
> > >
> > > Jordan
> > >
> > > On Tue, Mar 10, 2020 at 10:33 AM Benjamin Lerer <
> > > benjamin.le...@datastax.com>
> > > wrote:
> > >
> > > > +1
> > > >
> > > > On Tue, Mar 10, 2020 at 6:18 PM Jon Haddad 
> wrote:
> > > >
> > > > > I've used assertj in a lot of projects, I prefer it by a wide
> margin
> > > over
> > > > > using only junit.
> > > > >
> > > > > On Tue, Mar 10, 2020 at 9:45 AM David Capwell 
> > > > wrote:
> > > > >
> > > > > > +1 from me
> > > > > >
> > > > > > In CASSANDRA-15564 I build my own assert chain to make the tests
> > > > cleaner;
> > > > > > did it since assertj wasn't there.
> > > > > >
> > > > > > On Tue, Mar 10, 2020, 9:28 AM Kevin Gallardo <
> > > > > kevin.galla...@datastax.com>
> > > > > > wrote:
> > > > > >
> > > > > > > I would like to propose adding AssertJ <
> > > > >
> > > >
> > >
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__assertj.github.io_doc_&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=Jad7nE1Oab1mebx31r7AOfSsa0by8th6tCxpykmmOBA&m=WrkWi_LeOnAl7rqft1DM27OEkXD7sc2fnZMy_-c7IS8&s=D4FAaGRQi2WlwKAOQbQMfyt_cRqsOuZdePUDgchdLhA&e=
> > > > > >
> > > > > > as
> > > > > > > a test dependency and therefore have it available for writing
> > > > > > > unit/distributed/any test assertions.
> > > > > > >
> > > > > > > In addition to the examples mentioned on the AssertJ docs page
> > > > (allows
> > > > > to
> > > > > > > do elaborate and comprehensible assertions on Collections,
> > String,
> > > > and
> > > > > > > *custom
> > > > > > > assertions*), here's an example of a dtest I was looking at,
> that
> > > > could
> > > > > > be
> > > > > > > translated to AssertJ syntax, just to give an idea of how the
> > > syntax
> > > > > > would
> > > > > > > apply:
> > > > > > >
> > > > > > > *JUnit asserts*:
> > > > > > > try {
> > > > > > >[...]
> > > > > > > } catch (Exception e) {
> > > > > > > Assert.assertTrue(e instanceof RuntimeException);
> > > > > > > RuntimeException re = ((RuntimeException) e);
> > > > > > > Assert.assertTrue(re.getCause() instanceof
> > > ReadTimeoutException);
> > > > > > > ReadTimeoutException rte = ((ReadTimeoutException)
> > > e.getCause());
> > > > > > > Assert.assertTrue(rte.getMessage().contains("blabla")
> > > > > > >   &&
> rte.getMessage().contains("andblablo"));
> > > > > > > }
> > > > > > >
> > > > > > > *AssertJ style:*
> > > > > > > try {
> > > > > > > [...]
> > > > > > > } catch (Exception e) {
> > > > > > > Assertions.assertThat(e)
> > > > > > > .isInstanceOf(RuntimeException.class)
> > > > > > >
> >  .hasCauseExactlyInstanceOf(ReadTimeoutException.class)
> > > > > > > .hasMessageContaining("blabla")
> > > > > > > .hasMessageContaining("andblablo");
> > > > > > > }
> > > > > > >
> > > > > > > The syntax is more explicit and more comprehensible, but more
> > > > > > importantly,
> > > > > > > when one of the JUnit assertTrue() fails, you don't know *why*,
> > you
> > > > > only
> > > > > > > know that the resulting boolean expression is false.
> > > > > > > If a failure happened with the assertJ tests, the failure would
> > say
> > > > > > > "Exception
> > > > > > > did not contain expected message, expected "blabla", actual
> > > > > "notblabla""
> > > > > > > (same for a lot of other situations), this makes debugging a
> > > failure,
> > > > > > after
> > > > > > > a test ran and failed much easier. With JUnit asserts you would
> > > have
> > > > to
> > > > > > > additionally add a message explainin

Re: [proposal] Introduce AssertJ in test framework

[Stephen Mallette]

>  (4. potentially migrating other existing tests to AssertJ in the long
term)

Highest value conversions would probably be assertTrue/False() assertions
as I believe those offer the least feedback on failure. I would imagine
that exception assertions would follow that especially when leaning on
@Test(expected = ...) sort of syntax or using explicit try/catch with
fail() logic in tests. AssertJ exception assertions tend to be much more
readable with test intention more clear.

On Wed, Mar 11, 2020 at 10:59 AM Kevin Gallardo 
wrote:

> Hi,
>
> Once you get to know assertJ, it's impossible to go back to basic
> > assertions of JUnit
>
>
> Definitely with you on that :)
>
> Great to see people are excited about this, thanks for the responses. Given
> the positive feedback I have created CASSANDRA-15631
>  as a first step.
>
> My proposal to try to get this going smoothly would be:
>
> 1. introduce the dependency and rewrite an existing test suite to showcase
> it as a reference
> 2. encourage new tests to be written with AssertJ once the dep is added
> 3. add a mention of the preferred assertion library in the contributing
> guidelines and link to the reference for an example
> (4. potentially migrating other existing tests to AssertJ in the long term)
>
> wdyt?
>
>
> On Tue, Mar 10, 2020 at 6:24 PM Joshua McKenzie 
> wrote:
>
> > Strong +1 here. Many of you know I'm a C# / LINQ junkie though. ;)
> >
> > On Tue, Mar 10, 2020 at 3:55 PM DuyHai Doan 
> wrote:
> >
> > > Definitely +1
> > >
> > > Coding in Java every day, I can't write test without assertJ. Once you
> > get
> > > to know assertJ, it's impossible to go back to basic assertions of
> JUnit
> > > that feels really boilerplate
> > >
> > >
> > >
> > > On Tue, Mar 10, 2020 at 8:53 PM Jordan West 
> wrote:
> > >
> > > > If it encourages more  and higher quality test writing +1 (nb). Also,
> > low
> > > > risk given it’s a test dep.
> > > >
> > > > Using QuickTheories as an example, merging it with a new or updated
> > test
> > > > could be a good way to get it merged
> > > >
> > > > Jordan
> > > >
> > > > On Tue, Mar 10, 2020 at 10:33 AM Benjamin Lerer <
> > > > benjamin.le...@datastax.com>
> > > > wrote:
> > > >
> > > > > +1
> > > > >
> > > > > On Tue, Mar 10, 2020 at 6:18 PM Jon Haddad 
> > wrote:
> > > > >
> > > > > > I've used assertj in a lot of projects, I prefer it by a wide
> > margin
> > > > over
> > > > > > using only junit.
> > > > > >
> > > > > > On Tue, Mar 10, 2020 at 9:45 AM David Capwell <
> dcapw...@gmail.com>
> > > > > wrote:
> > > > > >
> > > > > > > +1 from me
> > > > > > >
> > > > > > > In CASSANDRA-15564 I build my own assert chain to make the
> tests
> > > > > cleaner;
> > > > > > > did it since assertj wasn't there.
> > > > > > >
> > > > > > > On Tue, Mar 10, 2020, 9:28 AM Kevin Gallardo <
> > > > > > kevin.galla...@datastax.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > I would like to propose adding AssertJ <
> > > > > >
> > > > >
> > > >
> > >
> >
> https://urldefense.proofpoint.com/v2/url?u=https-3A__assertj.github.io_doc_&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=Jad7nE1Oab1mebx31r7AOfSsa0by8th6tCxpykmmOBA&m=WrkWi_LeOnAl7rqft1DM27OEkXD7sc2fnZMy_-c7IS8&s=D4FAaGRQi2WlwKAOQbQMfyt_cRqsOuZdePUDgchdLhA&e=
> > > > > > >
> > > > > > > as
> > > > > > > > a test dependency and therefore have it available for writing
> > > > > > > > unit/distributed/any test assertions.
> > > > > > > >
> > > > > > > > In addition to the examples mentioned on the AssertJ docs
> page
> > > > > (allows
> > > > > > to
> > > > > > > > do elaborate and comprehensible assertions on Collections,
> > > String,
> > > > > and
> > > > > > > > *custom
> > > > > > > > assertions*), here's an example of a dtest I was looking at,
> > that
> > > > > could
> > > > > > > be
> > > > > > > > translated to AssertJ syntax, just to give an idea of how the
> > > > syntax
> > > > > > > would
> > > > > > > > apply:
> > > > > > > >
> > > > > > > > *JUnit asserts*:
> > > > > > > > try {
> > > > > > > >[...]
> > > > > > > > } catch (Exception e) {
> > > > > > > > Assert.assertTrue(e instanceof RuntimeException);
> > > > > > > > RuntimeException re = ((RuntimeException) e);
> > > > > > > > Assert.assertTrue(re.getCause() instanceof
> > > > ReadTimeoutException);
> > > > > > > > ReadTimeoutException rte = ((ReadTimeoutException)
> > > > e.getCause());
> > > > > > > > Assert.assertTrue(rte.getMessage().contains("blabla")
> > > > > > > >   &&
> > rte.getMessage().contains("andblablo"));
> > > > > > > > }
> > > > > > > >
> > > > > > > > *AssertJ style:*
> > > > > > > > try {
> > > > > > > > [...]
> > > > > > > > } catch (Exception e) {
> > > > > > > > Assertions.assertThat(e)
> > > > > > > > .isInstanceOf(RuntimeException.class)
> > > > > > > >
> > >  .hasCauseExactlyInstanceOf(ReadTimeoutException.class)
> > > > > > > > .hasMessageContaining("blabla")
> >