Skip to content

JAVA-3076: NullSavingStrategyIT sometimes fails with ProtocolError: M… #1669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hhughes merged 1 commit into from
Aug 21, 2023
Merged

JAVA-3076: NullSavingStrategyIT sometimes fails with ProtocolError: M… #1669

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 48 additions & 15 deletions integration-tests/src/test/java/com/datastax/oss/driver/mapper/NullSavingStrategyIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,11 @@
import com.datastax.oss.driver.api.mapper.entity.saving.NullSavingStrategy;
import com.datastax.oss.driver.api.testinfra.ccm.CcmRule;
import com.datastax.oss.driver.api.testinfra.session.SessionRule;
import com.datastax.oss.driver.api.testinfra.session.SessionUtils;
import com.datastax.oss.driver.categories.ParallelizableTests;
import java.util.Objects;
import java.util.UUID;
import org.junit.AfterClass;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
Expand All @@ -51,13 +53,24 @@ public class NullSavingStrategyIT {

private static final CcmRule CCM_RULE = CcmRule.getInstance();

private static final SessionRule<CqlSession> SESSION_RULE =
SessionRule.builder(CCM_RULE)
.withConfigLoader(
DriverConfigLoader.programmaticBuilder()
.withString(DefaultDriverOption.PROTOCOL_VERSION, "V3")
.build())
.build();
private static final SessionRule<CqlSession> SESSION_RULE = SessionRule.builder(CCM_RULE).build();

// JAVA-3076: V3 protocol calls that could trigger cassandra to issue client warnings appear to be
// inherently unstable when used at the same time as V4+ protocol clients (common since this is
// part of the parallelizable test suite).
//
// For this test we'll use latest protocol version for SessionRule set-up, which creates the
// keyspace and could potentially result in warning about too many keyspaces, and then create a
// new client for the tests to use, which they access via the static InventoryMapper instance
// `mapper`.
//
// This additional client is created in the @BeforeClass method #setup() and guaranteed to be
// closed in @AfterClass method #teardown().
//
// Note: The standard junit runner executes rules before class/test setup so the order of
// execution will be CcmRule#before > SessionRule#before > NullSavingStrategyIT#setup, meaning
// CCM_RULE/SESSION_RULE should be fully initialized by the time #setup() is invoked.
private static CqlSession v3Session;
Copy link
Contributor

@absurdfarce absurdfarce Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to why we're keeping this session around as a static reference when we really only need it for a one-off operation.

Copy link
Contributor Author

@hhughes hhughes Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to close this session at the end of the test class, for SESSION_RULE this is handled automatically by junit which calls SessionRule#after() (and subsequently session.close()) because it's a @ClassRule

Copy link
Contributor Author

@hhughes hhughes Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the comments could be clearer here - this session instance is being used in all the tests via the mapper object so we need to keep it around until after they've all finished


@ClassRule
public static final TestRule CHAIN = RuleChain.outerRule(CCM_RULE).around(SESSION_RULE);
Expand All @@ -66,14 +79,34 @@ public class NullSavingStrategyIT {

@BeforeClass
public static void setup() {
CqlSession session = SESSION_RULE.session();
session.execute(
SimpleStatement.builder(
"CREATE TABLE product_simple(id uuid PRIMARY KEY, description text)")
.setExecutionProfile(SESSION_RULE.slowProfile())
.build());

mapper = new NullSavingStrategyIT_InventoryMapperBuilder(session).build();
// setup table for use in tests, this can use the default session
SESSION_RULE
.session()
.execute(
SimpleStatement.builder(
"CREATE TABLE product_simple(id uuid PRIMARY KEY, description text)")
.setExecutionProfile(SESSION_RULE.slowProfile())
.build());

// Create V3 protocol session for use in tests, will be closed in #teardown()
v3Session =
SessionUtils.newSession(
CCM_RULE,
SESSION_RULE.keyspace(),
Copy link
Contributor

@absurdfarce absurdfarce Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interesting question (that I genuinely don't know the answer to)... is this always safe? We're accessing a static field from a static method which presumes that the field will always be initialized before the method is called... even though both are static. This isn't a change in behaviour (the old code did it too)... just something I wondered about.

Copy link
Contributor Author

@hhughes hhughes Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the junit runner. In this case we're using the default which ends up being ParentRunner. Jumping into the debugger that uses this classBlock() method to determine the order of operation:

If there are any children remaining after filtering and ignoring, construct a statement that will:

  • Apply all ClassRules on the test-class and superclasses.
  • Run all non-overridden @BeforeClass methods on the test-class and superclasses; if any throws an Exception, stop execution and pass the exception on.
  • Run all remaining tests on the test-class.
  • Run all non-overridden @AfterClass methods on the test-class and superclasses: exceptions thrown by previous steps are combined, if necessary, with exceptions from AfterClass methods into a [MultipleFailureException]

https://junit.org/junit4/javadoc/4.13/org/junit/runners/ParentRunner.html#classBlock(org.junit.runner.notification.RunNotifier)

So the order will be CcmRule#before() > SessionRule#before() > NullSavingStrategyIT#setup()

DriverConfigLoader.programmaticBuilder()
.withString(DefaultDriverOption.PROTOCOL_VERSION, "V3")
.build());

// Hand V3 session to InventoryMapper which the tests will use to perform db calls
mapper = new NullSavingStrategyIT_InventoryMapperBuilder(v3Session).build();
}
Copy link
Contributor

@absurdfarce absurdfarce Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comment above... doesn't this accomplish exactly the same thing without requiring a new field (or a teardown method)?

  @BeforeClass
  public static void setup() {
    
    // JAVA-3076: Perform setup using a one-off v5 session to avoid any errors when using v3
    DriverConfigLoader setupSessionLoader = DriverConfigLoader.programmaticBuilder()
            .withString(DefaultDriverOption.PROTOCOL_VERSION, "V5")
            .build());
    try (CqlSession setupSession = SessionUtils.newSession(CCM_RULE, SESSION_RULE.keyspace(), setupSessionLoader)) {

      setupSession.execute(
              SimpleStatement.builder(
                              "CREATE TABLE product_simple(id uuid PRIMARY KEY, description text)")
                      .setExecutionProfile(SESSION_RULE.slowProfile())
                      .build());
    }

    // JAVA-3076: Actual test should continue to use v3 just like it always has
    mapper = new NullSavingStrategyIT_InventoryMapperBuilder(SESSION_RULE.session()).build();
  }

The code snippet above assumes that SESSION_RULE remains identical to what it current is in 4.x.

Copy link
Contributor Author

@hhughes hhughes Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error isn't coming from the table create in the setup() call but the keyspace creation SessionRule#before() - which is invoked by junit as part of @ClassRule - so I don't the snippet fixes this issue. I'll rework the comments in this file to make that clearer


@AfterClass
public static void teardown() {
// Close V3 session (SESSION_RULE will be closed separately by @ClassRule handling)
if (v3Session != null) {
v3Session.close();
}
}

@Test
Expand Down