Skip to content
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.

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

Wrong handling of multiple consecutive empty pages in Java Driver 4.x #254

Closed
avelanarius opened this issue Nov 2, 2023 · 12 comments
Closed
Assignees

Comments

@avelanarius
Copy link

When Scylla returns multiple consecutive empty pages, Java Driver 4.x doesn't handle it correctly. Reproducer:

CqlSession cqlSession = CqlSession.builder()
        .addContactPoint(InetSocketAddress.createUnresolved("127.0.0.2", 9042))
        .withLocalDatacenter("datacenter1").build();

cqlSession.execute("CREATE KEYSPACE IF NOT EXISTS ks WITH REPLICATION = {'class': 'SimpleStrategy', 'replication_factor': 1}");

cqlSession.execute("CREATE TABLE IF NOT EXISTS ks.t (pk int, ck int, v int, PRIMARY KEY(pk, ck))");

for (int i = 0; i < 50; i++) {
    System.out.println("Insert " + i);
    cqlSession.execute("INSERT INTO ks.t(pk, ck, v) VALUES (?, ?, ?)", i, i, 15);
    cqlSession.execute("INSERT INTO ks.t(pk, ck, v) VALUES (?, ?, ?)", i, i, 32);
}

cqlSession.execute("INSERT INTO ks.t(pk, ck, v) VALUES (?, ?, ?)", 8, 8, 14);
cqlSession.execute("INSERT INTO ks.t(pk, ck, v) VALUES (?, ?, ?)", 11, 11, 14);
cqlSession.execute("INSERT INTO ks.t(pk, ck, v) VALUES (?, ?, ?)", 14, 14, 14);

SimpleStatement st = SimpleStatement.newInstance("SELECT * FROM ks.t WHERE v = 14 ALLOW FILTERING");
st = st.setPageSize(1);

cqlSession.execute(st).all().forEach(r -> {
    System.out.println(r);
});
cqlSession.close();

The code is supposed to print 3 rows, but it doesn't print anything.

@avelanarius
Copy link
Author

avelanarius commented Nov 2, 2023

Maybe this is a wrong repo to write this, but as for other drivers:

  • Rust Driver does not have this problem.
  • Java Driver 3.x does not have this problem.

@vladzcloudius
Copy link

vladzcloudius commented Nov 3, 2023

First of all I'm not sure the "expected behavior" is correct: you set the page size to 1 and execute the query just once. AFAIR this way you are supposed to get a single page only.
And as such you should be getting a single row at most. Don't you?

If I'm right then the behavior of the 4.x driver is actually right (it apparently brings you a single empty page) while the behavior of other drivers isn't.

Or possibly the semantics of execute() got changed between drivers and Java driver versions:

  • It 4.x it brings you a single page.
  • In 3.x and in Rust - brings you all pages (which is kinda weird and seems bogus).

@avelanarius
Copy link
Author

First of all I'm not sure the "expected behavior" is correct: you set the page size to 1 and execute the query just once. AFAIR this way you are supposed to get a single page only. And as such you should be getting a single row at most. Don't you?

No, the driver is supposed to transparently fetch new pages as you iterate (https://docs.datastax.com/en/developer/java-driver/4.13/manual/core/paging/#quick-overview).

@vladzcloudius
Copy link

First of all I'm not sure the "expected behavior" is correct: you set the page size to 1 and execute the query just once. AFAIR this way you are supposed to get a single page only. And as such you should be getting a single row at most. Don't you?

No, the driver is supposed to transparently fetch new pages as you iterate (https://docs.datastax.com/en/developer/java-driver/4.13/manual/core/paging/#quick-overview).

Got it. Then there is some bug indeed.
I remembered that you needed some combo of hasMorePages(...) and execute() in a loop to fetch results. But I see that they hid these in the iterator these days.
Thanks for explaining.

@roydahan
Copy link
Collaborator

@avelanarius are you sending a PR with the fix (avelanarius@40da47a)?
(Unless you already did and I missed it.

@sylwiaszunejko
Copy link

sylwiaszunejko commented Nov 15, 2023

Update about other drivers:

  • Python Driver does not have this problem.
  • gocql does not have this problem.

@mykaul
Copy link

mykaul commented Nov 15, 2023

  • gocql does have this problem.

Thanks @sylwiaszunejko - please open an issue on its repo so we can track it properly.

@sylwiaszunejko
Copy link

  • gocql does have this problem.

Thanks @sylwiaszunejko - please open an issue on its repo so we can track it properly.

Sorry for my mistake, I already edited the comment it does not have this problem

@mykaul
Copy link

mykaul commented Nov 15, 2023

  • gocql does have this problem.

Thanks @sylwiaszunejko - please open an issue on its repo so we can track it properly.

Sorry for my mistake, I already edited the comment it does not have this problem

Even better! So now lets just make sure Rust and Java are safe (and btw, would be good to add tests to them)

@roydahan
Copy link
Collaborator

@avelanarius are we sending PR to the upstream driver? to our fork? Both?

Bouncheck added a commit to Bouncheck/java-driver that referenced this issue Nov 20, 2023
Adds a simple test that checks if empty pages are correctly
skipped through and all rows are being returned.
Covers scylladb#254.
Bouncheck added a commit to Bouncheck/java-driver that referenced this issue Nov 20, 2023
Adds a test method to SimpleStatementCcmIT that checks
if SimpleStatement with page size 1 correctly fetches all rows.
Covers issue scylladb#254.
This was referenced Nov 20, 2023
@avelanarius avelanarius assigned Bouncheck and unassigned avelanarius Dec 4, 2023
Bouncheck added a commit to Bouncheck/java-driver that referenced this issue Dec 6, 2023
Adds a test method to SimpleStatementCcmIT that checks
if SimpleStatement with page size 1 correctly fetches all rows.
Covers issue scylladb#254.
avelanarius pushed a commit to Bouncheck/java-driver that referenced this issue Jan 4, 2024
Adds a simple test that checks if empty pages are correctly
skipped through and all rows are being returned.
Covers scylladb#254.
avelanarius pushed a commit to Bouncheck/java-driver that referenced this issue Jan 4, 2024
Adds a simple test that checks if empty pages are correctly
skipped through and all rows are being returned.
Covers scylladb#254.
avelanarius pushed a commit that referenced this issue Jan 5, 2024
Adds a simple test that checks if empty pages are correctly
skipped through and all rows are being returned.
Covers #254.
@mykaul
Copy link

mykaul commented Jan 8, 2024

@avelanarius - what's the latest status here?

avelanarius pushed a commit that referenced this issue Jan 15, 2024
Adds a test method to SimpleStatementCcmIT that checks
if SimpleStatement with page size 1 correctly fetches all rows.
Covers issue #254.
@avelanarius
Copy link
Author

Solved by merged a04fadf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants