Merge pull request #1935 from ohadzeliger/remote-fetch-byte-count

Resolve #1934: Fix bug in byte counting with remote fetch
FoundationDB · Dec 1, 2022 · a5f98c4 · a5f98c4
2 parents f03fe0c + 922d051
commit a5f98c4
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 12 deletions.
diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md
@@ -15,7 +15,7 @@ The Guava dependency version has been updated to 31.1. Projects may need to chec
 // begin next release
 ### NEXT_RELEASE
 
-* **Bug fix** Fix 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
+* **Bug fix** Fix byte counting bug for remote fetch [(Issue #1934)](https://github.com/FoundationDB/fdb-record-layer/issues/1934)
 * **Bug fix** Fix 2 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 3 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
 * **Bug fix** Fix 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)

diff --git a/...ore/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/...ore/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java
@@ -1311,7 +1311,7 @@ protected <M extends Message> CompletableFuture<FDBIndexedRecord<M>> buildSingle
             versionFutureOptional = loadRecordVersionAsync(indexedRawRecord.getIndexEntry().getPrimaryKey());
         }
         if (byteScanLimiter != null) {
-            byteScanLimiter.registerScannedBytes(indexedRawRecord.getIndexEntry().getKeySize());
+            // Only count the bytes of the RawRecord - the index entry bytes are accounted for by the KeyValueCursor
             byteScanLimiter.registerScannedBytes((long)sizeInfo.getKeySize() + (long)sizeInfo.getValueSize());
         }
 

diff --git a/...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/RemoteFetchTest.java b/...re/src/test/java/com/apple/foundationdb/record/provider/foundationdb/RemoteFetchTest.java
@@ -41,7 +41,6 @@
 import com.apple.test.Tags;
 import com.google.protobuf.Message;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -243,27 +242,30 @@ void indexPrefetchWithMixedContinuationTest() throws Exception {
 
     @ParameterizedTest(name = "indexPrefetchByteLimitContinuation(" + ARGUMENTS_WITH_NAMES_PLACEHOLDER + ")")
     @EnumSource()
-    @Disabled("This test is inconsistently failing when running as part of the larger suite")
     void indexPrefetchByteLimitContinuation(IndexFetchMethod useIndexPrefetch) throws Exception {
         RecordQueryPlan plan = plan(NUM_VALUES_LARGER_THAN_990, useIndexPrefetch);
-        // TODO: Why should the index prefetch take so many more bytes to scan the same number of records? Maybe the index scan counts the records and the fetch does not?
-        int scanBytesLimit = (useIndexPrefetch == IndexFetchMethod.SCAN_AND_FETCH) ? 350 : 1300;
+        // There is a slight difference between the scan and remote fetch way of counting bytes. Scan starts a few fetches
+        // concurrently while remote fetch counts bytes that were already fetched. As a result, scan will continue to return
+        // records that arrive (even if they are above the limit), while remote fetch will stop as soon as the limit has
+        // been reached.
+        // For this test, we set the limit artificially small, to ensure it is honored (reached immediately, therefore
+        // returning one record).
         ExecuteProperties executeProperties = ExecuteProperties.newBuilder()
-                .setScannedBytesLimit(scanBytesLimit)
+                .setScannedBytesLimit(1)
                 .build();
-
-        byte[] continuation = executeAndVerifyData(plan, null, executeProperties, 8, (rec, i) -> {
+        byte[] continuation = executeAndVerifyData(plan, null, executeProperties, 1, (rec, i) -> {
             int primaryKey = 9 - i;
             String strValue = ((primaryKey % 2) == 0) ? "even" : "odd";
             int numValue = 1000 - primaryKey;
             assertRecord(rec, primaryKey, strValue, numValue, "MySimpleRecord$num_value_unique", (long)numValue, primaryKey);
         }, splitRecordsHook);
+        // Now set the limit high to allow for the rest of the records to be reasd
         executeProperties = ExecuteProperties.newBuilder()
-                .setScannedBytesLimit(500)
+                .setScannedBytesLimit(5000)
                 .build();
 
-        continuation = executeAndVerifyData(plan, continuation, executeProperties, 2, (rec, i) -> {
-            int primaryKey = 1 - i;
+        continuation = executeAndVerifyData(plan, continuation, executeProperties, 9, (rec, i) -> {
+            int primaryKey = 8 - i;
             String strValue = ((primaryKey % 2) == 0) ? "even" : "odd";
             int numValue = 1000 - primaryKey;
             assertRecord(rec, primaryKey, strValue, numValue, "MySimpleRecord$num_value_unique", (long)numValue, primaryKey);