This is an automated email from the ASF dual-hosted git repository.
xiangfu pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 72755a41327 [cleanup] Remove unnecessary backward-compat check in
DataTable serialization (#18002)
72755a41327 is described below
commit 72755a41327b3235d302527ae93b228838f69ea5
Author: Xiang Fu <[email protected]>
AuthorDate: Fri Mar 27 15:24:26 2026 -0700
[cleanup] Remove unnecessary backward-compat check in DataTable
serialization (#18002)
* Remove unnecessary backward-compat check in DataTable serialization
The conditional check on CPU time and memory measurement during DataTable
serialization was kept solely for backward compatibility per the inline
comment. All current server versions support thread resource usage
reporting, making this guard unnecessary.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
* Restore positive-value assertions when measurement is enabled
Keep strong assertions (> 0) for the enabled-measurement case.
Use assertNotNull for the disabled case where values are always
populated but may be zero. Improve comment accuracy.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
* Address review: keep conditional logic, fix misleading comment only
Per reviewer feedback, the conditional check on CPU time and memory
measurement should be kept — when these are not collectable, we should
not return their values. Reverted the production code change and updated
the comment to accurately describe the intended behavior instead of the
old misleading TODO.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
---------
Co-authored-by: Pinot Cleanup Agent <[email protected]>
Co-authored-by: Claude Opus 4.6 <[email protected]>
---
.../main/java/org/apache/pinot/common/datatable/DataTableImplV4.java | 5 ++---
.../org/apache/pinot/core/common/datatable/DataTableSerDeTest.java | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java
b/pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java
index fc9088ced89..64bda5d0f04 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java
@@ -422,9 +422,8 @@ public class DataTableImplV4 implements DataTable {
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.
+ // Add table serialization time and memory metadata when the corresponding
measurement is enabled.
+ // When CPU time/memory usage is not collectable, we omit these values
from the metadata.
if (ThreadResourceUsageProvider.isThreadCpuTimeMeasurementEnabled()) {
getMetadata().put(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName(),
String.valueOf(resourceSnapshot.getCpuTimeNs()));
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java
index 32eff10a16f..a3c8ed94ede 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java
@@ -280,7 +280,7 @@ public class DataTableSerDeTest {
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(true);
DataTable dataTable = dataTableBuilder.build();
DataTable newDataTable =
DataTableFactory.getDataTable(dataTable.toBytes());
- // When ThreadCpuTimeMeasurement is enabled, value of
responseSerializationCpuTimeNs is not 0.
+ // When ThreadCpuTimeMeasurement is enabled,
responseSerializationCpuTimeNs should be positive.
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()));
@@ -294,8 +294,7 @@ public class DataTableSerDeTest {
ThreadResourceUsageProvider.setThreadMemoryMeasurementEnabled(false);
dataTable = dataTableBuilder.build();
newDataTable = DataTableFactory.getDataTable(dataTable.toBytes());
- // When ThreadCpuTimeMeasurement is disabled, no value for
- //
threadCpuTimeNs/systemActivitiesCpuTimeNs/responseSerializationCpuTimeNs.
+ // When measurement is disabled, response serialization metadata is absent.
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()));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
