Server continuations #3030

ohadzeliger · 2025-01-10T18:20:37Z

No description provided.

foundationdb-ci · 2025-01-10T23:41:05Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 6c231d5
Duration 0:52:50
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

...nal-grpc/src/main/java/com/apple/foundationdb/relational/jdbc/RelationalRpcContinuation.java

...ational-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/AbstractMockResultSet.java

fdb-relational-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/MockContinuation.java

...ational-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/MockResultSetMetadata.java

...nal-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/ResultSetContinuationTest.java

ScottDugas · 2025-01-13T20:13:31Z

...nal-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/ResultSetContinuationTest.java

+        Assertions.assertEquals(expected.atBeginning(), actual.atBeginning());
+        Assertions.assertEquals(expected.atEnd(), actual.atEnd());
+        Assertions.assertEquals(expected.getReason(), actual.getReason());
+        // This, again, assumes that the entire inner continuation is serialized as the state


Why not call actual.serialize()?

Actual in this case is the RPC continuation (that wraps around the inner, server-side, continuation).
I can pull that out of the assert method to the test method, if that helps.

If it's the RPC continuation, aren't actual.serialize() and actual.getExecutionState() the same?
serialize() is the method that we use to send across the wire, and thus would get sent back in EXECUTE CONTINUATION right? So that should be what we assert about?

In this case the continuation is always RelationalRpcContinuation and so they are always the same. Done.

ScottDugas · 2025-01-13T20:15:01Z

...nal-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/ResultSetContinuationTest.java

+        ResultSet rsProto = TypeConversion.toProtobuf(resultSet);
+
+        try (RelationalResultSetFacade deserializedResultSet = new RelationalResultSetFacade(rsProto)) {
+            Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation());


This is only testing that it fails before getting any results. It seems like that is a good thing to test, but also that we should test after getting one.

This now asserts that it doesn't fail after getting all of them. I was thinking more like:

Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); assertTrue(convertedResultSet.next()); Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); toRow(convertedResultSet, numColumns); // row should be valid assertTrue(convertedResultSet.next()); toRow(convertedResultSet, numColumns); // row should be valid Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); assertFalse(convertedResultSet.next()); deserializedResultSet.getContinuation();

(Maybe with one more successful, and

Yes, that's a good enhancement. Done.
This was also used to cover the off-by-one bug in the result set facade that only allowed continuations after the last row.

ScottDugas · 2025-01-13T20:25:14Z

yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryConfig.java

@@ -154,7 +153,7 @@ private static String appendWithContinuationIfPresent(@Nonnull String queryStrin
        if (!currentQuery.isEmpty() && currentQuery.charAt(currentQuery.length() - 1) == ';') {
            currentQuery = currentQuery.substring(0, currentQuery.length() - 1);
        }
-        if (continuation instanceof ContinuationImpl) {
+        if (continuation != null) {


Should we be adding any sort of assertions to the yaml test framework about the reason for the continuation?
Should we assert that we never get back the beginning continuation?
I don't think the yaml tests have a good way to cause TRANSACTION_LIMIT_REACHED, but the others should be testable in an integrated way, which seems worthwhile.

These are good enhancements. The unit tests do have assertions for that (see #3038 ) but YAML is somewhat limited in handling continuations (all is implicit).

Created a ticket

foundationdb-ci · 2025-01-14T19:44:58Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: fffc169
Duration 0:55:30
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

ScottDugas · 2025-01-14T19:56:07Z

fdb-relational-grpc/src/main/proto/grpc/relational/jdbc/v1/result_set.proto

@@ -44,7 +44,7 @@ message ResultSet {
  repeated Struct row = 2;

  // Optional continuation that can continue the query from the point the current results ended.
-  // Not all queries support continuations. Null (or non existent) continuations means "no continuation".
+  // A result set is non-null for the RelationalResultSet API but is not always supported (exception thrown when not).


Did you mean "continuation"?

Suggested change

// A result set is non-null for the RelationalResultSet API but is not always supported (exception thrown when not).

// A continuation is non-null for the RelationalResultSet API but is not always supported (exception thrown when not).

ScottDugas · 2025-01-14T20:30:27Z

...nal-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/ResultSetContinuationTest.java

+        Assertions.assertEquals(expected.atBeginning(), actual.atBeginning());
+        Assertions.assertEquals(expected.atEnd(), actual.atEnd());
+        Assertions.assertEquals(expected.getReason(), actual.getReason());
+        // This, again, assumes that the entire inner continuation is serialized as the state


If it's the RPC continuation, aren't actual.serialize() and actual.getExecutionState() the same?
serialize() is the method that we use to send across the wire, and thus would get sent back in EXECUTE CONTINUATION right? So that should be what we assert about?

ScottDugas · 2025-01-14T20:54:28Z

...nal-grpc/src/test/java/com/apple/foundationdb/relational/jdbc/ResultSetContinuationTest.java

+        ResultSet rsProto = TypeConversion.toProtobuf(resultSet);
+
+        try (RelationalResultSetFacade deserializedResultSet = new RelationalResultSetFacade(rsProto)) {
+            Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation());


This now asserts that it doesn't fail after getting all of them. I was thinking more like:

Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); assertTrue(convertedResultSet.next()); Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); toRow(convertedResultSet, numColumns); // row should be valid assertTrue(convertedResultSet.next()); toRow(convertedResultSet, numColumns); // row should be valid Assertions.assertThrows(SQLException.class, () -> deserializedResultSet.getContinuation()); assertFalse(convertedResultSet.next()); deserializedResultSet.getContinuation();

(Maybe with one more successful, and

ScottDugas · 2025-01-14T20:59:25Z

yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/command/QueryConfig.java

@@ -225,6 +224,9 @@ void checkResultInternal(@Nonnull Object actual, @Nonnull String queryDescriptio
                var success = isExact ? getVal().equals(actualPlan) : actualPlan.contains((String) getVal());
                if (success) {
                    logger.debug("✅️ plan match!");
+                    while (resultSet.next()) {


Yes, but why would there be more results? Shouldn't an explain always return one?

foundationdb-ci · 2025-01-14T23:17:22Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 898846b
Duration 0:55:51
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-14T23:21:53Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 4db6a16
Duration 0:01:38
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-15T00:00:40Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 6133e05
Duration 0:56:02
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

# Conflicts: # fdb-relational-grpc/fdb-relational-grpc.gradle

foundationdb-ci · 2025-01-15T03:56:48Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: 6133e05
Duration 0:36:53
Result: ❌ FAILED
Error: Error while executing command: ./gradlew --no-daemon --console=plain -b ./build.gradle build destructiveTest -PcoreNotStrict -PreleaseBuild=false -PpublishBuild=false -PspotbugsEnableHtmlReport. Reason: exit status 1
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

foundationdb-ci · 2025-01-15T04:22:18Z

Result of fdb-record-layer-pr on Linux CentOS 7

Commit ID: f8daa8f
Duration 0:53:19
Result: ✅ SUCCEEDED
Error: N/A
Build Log terminal output (available for 30 days)
Build Workspace zip file of the working directory (available for 30 days)

ohadzeliger added 10 commits January 10, 2025 13:19

Initial commit for client-server support for continuations

898a937

Move continuation to separate proto file, store in response

e501c68

Undo changes that are not needed

3f3d670

checkstyle

e546bd3

Add tests, handle nul and error cases

c3c1fd4

Add tests

4f77110

Fail continuation if the result set is not exhausted

967c306

Add dependencies from previous PRs

1170161

Exhaust the result set in the check explain

1198f12

PMD

6c231d5

ohadzeliger marked this pull request as ready for review January 11, 2025 00:46

ohadzeliger requested a review from ScottDugas January 11, 2025 00:46

ScottDugas requested changes Jan 13, 2025

View reviewed changes

ohadzeliger added 3 commits January 14, 2025 12:30

PR Comments

966595c

PR Comments

889fdd4

PR Comments

fffc169

ohadzeliger requested a review from ScottDugas January 14, 2025 18:57

ScottDugas requested changes Jan 14, 2025

View reviewed changes

ohadzeliger added 2 commits January 14, 2025 17:21

PR Comments

898846b

PR Comments

6133e05

ohadzeliger requested a review from ScottDugas January 14, 2025 23:17

FoundationDB deleted a comment from foundationdb-ci Jan 14, 2025

ohadzeliger force-pushed the server-continuations branch from 4db6a16 to 6133e05 Compare January 15, 2025 03:19

ohadzeliger added 2 commits January 14, 2025 22:21

Merge branch 'refs/heads/main' into server-continuations

16d8a36

# Conflicts: # fdb-relational-grpc/fdb-relational-grpc.gradle

Rebase from main

f8daa8f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server continuations #3030

Server continuations #3030

ohadzeliger commented Jan 10, 2025

foundationdb-ci commented Jan 10, 2025

ScottDugas Jan 13, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 14, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 13, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 14, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 13, 2025

ohadzeliger Jan 14, 2025

ohadzeliger Jan 14, 2025

foundationdb-ci commented Jan 14, 2025

ScottDugas Jan 14, 2025

ohadzeliger Jan 14, 2025

ScottDugas Jan 14, 2025

ScottDugas Jan 14, 2025

ScottDugas Jan 14, 2025

foundationdb-ci commented Jan 14, 2025

foundationdb-ci commented Jan 14, 2025

foundationdb-ci commented Jan 15, 2025

foundationdb-ci commented Jan 15, 2025

foundationdb-ci commented Jan 15, 2025

	// A result set is non-null for the RelationalResultSet API but is not always supported (exception thrown when not).
	// A continuation is non-null for the RelationalResultSet API but is not always supported (exception thrown when not).

Server continuations #3030

Are you sure you want to change the base?

Server continuations #3030

Conversation

ohadzeliger commented Jan 10, 2025

foundationdb-ci commented Jan 10, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foundationdb-ci commented Jan 14, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foundationdb-ci commented Jan 14, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 14, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7

foundationdb-ci commented Jan 15, 2025

Result of fdb-record-layer-pr on Linux CentOS 7