Copilot commented on code in PR #18002:
URL: https://github.com/apache/pinot/pull/18002#discussion_r3002767128
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java:
##########
@@ -422,17 +421,11 @@ public byte[] toBytes()
DataOutputStream dataOutputStream = new
DataOutputStream(byteArrayOutputStream);
writeLeadingSections(dataOutputStream);
- // Add table serialization time and memory metadata if thread timer is
enabled.
- // TODO: The check on cpu time and memory measurement is not needed. We
can remove it. But keeping it around for
- // backward compatibility.
- if (ThreadResourceUsageProvider.isThreadCpuTimeMeasurementEnabled()) {
- getMetadata().put(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName(),
- String.valueOf(resourceSnapshot.getCpuTimeNs()));
- }
- if (ThreadResourceUsageProvider.isThreadMemoryMeasurementEnabled()) {
- getMetadata().put(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName(),
- String.valueOf(resourceSnapshot.getAllocatedBytes()));
- }
+ // Add table serialization time and memory metadata.
+ getMetadata().put(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName(),
+ String.valueOf(resourceSnapshot.getCpuTimeNs()));
+ getMetadata().put(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName(),
+ String.valueOf(resourceSnapshot.getAllocatedBytes()));
Review Comment:
Now that response serialization metadata is always emitted, the comment
should document the expected values when measurement is disabled/unavailable
(e.g., zero-filled). Adding that note here (or on the corresponding
`MetadataKey` docs) helps downstream consumers understand whether to treat `0`
differently from missing values, especially since this PR changes the old
absence-based behavior.
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -294,13 +294,12 @@ public void testThreadCPUMemMeasurement(int
dataTableVersion) throws IOException
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(false);
dataTable = dataTableBuilder.build();
newDataTable = DataTableFactory.getDataTable(dataTable.toBytes());
- // When ThreadCpuTimeMeasurement is disabled, no value for
- //
threadCpuTimeNs/systemActivitiesCpuTimeNs/responseSerializationCpuTimeNs.
+ // Response serialization metadata is always populated, but may be zero
when measurement is disabled.
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_CPU_TIME_NS.getName()));
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName()));
-
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName()));
+
Assert.assertNotNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName()));
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_MEM_ALLOCATED_BYTES.getName()));
-
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName()));
+
Assert.assertNotNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName()));
Review Comment:
Since the intent is that these fields are always populated but might be zero
when measurement is disabled, asserting only `NotNull` is relatively weak.
Consider also validating that the values are numeric (parseable as `long`) and
match the expected disabled semantics (e.g., `== 0` if guaranteed, or `>= 0` if
not). This makes the test more robust and ensures the serialization contract is
enforced.
##########
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:
##########
@@ -280,7 +280,7 @@ public void testThreadCPUMemMeasurement(int
dataTableVersion) throws IOException
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(true);
DataTable dataTable = dataTableBuilder.build();
DataTable newDataTable =
DataTableFactory.getDataTable(dataTable.toBytes());
- // When ThreadCpuTimeMeasurement is enabled, value of
responseSerializationCpuTimeNs is not 0.
+ // When measurement is enabled, response serialization metadata should
have positive values.
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_CPU_TIME_NS.getName()));
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName()));
Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_MEM_ALLOCATED_BYTES.getName()));
Review Comment:
The updated comment says response serialization metadata should have
positive values when measurement is enabled, but the test does not assert
anything about `RESPONSE_SER_CPU_TIME_NS` / `RESPONSE_SER_MEM_ALLOCATED_BYTES`
in this enabled branch. Add assertions that these keys are present and validate
the values (e.g., parse as long and assert > 0) or adjust the comment to match
what the test actually verifies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
