diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md index a6bb53bb0f..55994326cd 100644 --- 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/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 07b83039a3..6971b20bdc 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1311,7 +1311,7 @@ protected CompletableFuture> 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/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/RemoteFetchTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/RemoteFetchTest.java index 3d20f24c6d..152e5c871e 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/RemoteFetchTest.java +++ b/fdb-record-layer-core/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);