xiangfu0 commented on code in PR #19861:
URL: https://github.com/apache/pinot/pull/19861#discussion_r4289854236
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ControllerJobStatusResource.java:
##########
@@ -85,9 +85,15 @@ public String
reloadJobStatus(@PathParam("tableNameWithType") String tableNameWi
totalSegmentCount = targetSegments.size();
}
try {
+ ReloadJobStatus jobStatus = reloadJobId != null ?
_serverReloadJobStatusCache.getJobStatus(reloadJobId) : null;
+ // Segments skipped by an only-if-needed reload keep their old load time
but are up to date, so they count as
+ // successful. Checking by name counts each segment once, even if it was
reloaded again after being skipped, and
+ // leaves out skipped segments that have since been dropped.
+ boolean hasSkippedSegments = jobStatus != null &&
jobStatus.hasSkippedSegments();
int successCount = 0;
for (SegmentDataManager segmentDataManager : segmentDataManagers) {
- if (segmentDataManager.getLoadTimeMs() >=
reloadJobSubmissionTimestamp) {
+ if (segmentDataManager.getLoadTimeMs() >= reloadJobSubmissionTimestamp
+ || (hasSkippedSegments &&
jobStatus.isSkipped(segmentDataManager.getSegmentName()))) {
Review Comment:
For a skipped segment, success is signalled only through this `isSkipped`
lookup, since its `loadTimeMs` stays older than the job timestamp. That signal
lives solely in the Guava cache (expireAfterWrite + maxSize), unlike a normal
reload whose `loadTimeMs` is durable.
So once the entry is evicted (default 30d TTL, or >maxSize jobs, or a config
rebuild), a re-query counts those segments as pending again and the job reports
IN_PROGRESS forever. A normal reload never flips back like this. Could we make
the skip durable (e.g. bump loadTimeMs, or treat a missing-but-dispatched job
as complete) or at least document the caveat?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java:
##########
@@ -169,15 +169,41 @@ void deleteSegment(String segmentName)
/// `forceDownload` flag is true.
/// This operation is conducted within a failure handling framework and made
transparent to ongoing queries, because
/// the segment is in online serving state.
- void reloadSegment(String segmentName, boolean forceDownload, String
reloadJobId)
+ default void reloadSegment(String segmentName, boolean forceDownload, String
reloadJobId)
+ throws Exception {
+ reloadSegment(segmentName, forceDownload, false, reloadJobId);
+ }
+
+ /// Same as [#reloadSegment(String, boolean, String)], but when
`onlyIfNeeded` is true (and `forceDownload` is false)
+ /// the segment is skipped if its loaded copy is already up to date: same
CRC as ZK, already on the target tier, and
+ /// no preprocessing needed under the current table config and schema.
Consuming segments are always skipped in this
+ /// mode. Skipped segments are recorded in the reload job status and
reported as successful.
+ ///
+ /// This does not cover settings that only take effect when a segment is
loaded (e.g. read mode or tier storage
+ /// properties) or rebuilding derived state such as upsert metadata; a full
reload is still needed for those.
+ void reloadSegment(String segmentName, boolean forceDownload, boolean
onlyIfNeeded, String reloadJobId)
Review Comment:
This breaks external implementers of this SPI. Turning the old 3-arg methods
into `default` while adding these new *abstract* overloads means any
out-of-tree `TableDataManager` (plugged in via
`table.data.manager.provider.class`) that only implemented the 3-arg versions
will no longer compile, and an already-built one throws `AbstractMethodError`
since the server now always calls the 4-arg form.
Since `japicmp.skip=true`, CI won't catch this. Consider giving the new
overloads a `default` body that falls back to the old behavior (ignoring
`onlyIfNeeded`) and overriding them in `BaseTableDataManager` — that keeps
existing implementers working. Same applies to `InstanceDataManager`.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/ControllerJobStatusResource.java:
##########
@@ -85,9 +85,15 @@ public String
reloadJobStatus(@PathParam("tableNameWithType") String tableNameWi
totalSegmentCount = targetSegments.size();
}
try {
+ ReloadJobStatus jobStatus = reloadJobId != null ?
_serverReloadJobStatusCache.getJobStatus(reloadJobId) : null;
+ // Segments skipped by an only-if-needed reload keep their old load time
but are up to date, so they count as
+ // successful. Checking by name counts each segment once, even if it was
reloaded again after being skipped, and
+ // leaves out skipped segments that have since been dropped.
+ boolean hasSkippedSegments = jobStatus != null &&
jobStatus.hasSkippedSegments();
int successCount = 0;
for (SegmentDataManager segmentDataManager : segmentDataManagers) {
- if (segmentDataManager.getLoadTimeMs() >=
reloadJobSubmissionTimestamp) {
+ if (segmentDataManager.getLoadTimeMs() >= reloadJobSubmissionTimestamp
+ || (hasSkippedSegments &&
jobStatus.isSkipped(segmentDataManager.getSegmentName()))) {
Review Comment:
For a skipped segment, success is signalled only through this `isSkipped`
lookup, since its `loadTimeMs` stays older than the job timestamp. That signal
lives solely in the Guava cache (expireAfterWrite + maxSize), unlike a normal
reload whose `loadTimeMs` is durable.
So once the entry is evicted (default 30d TTL, or >maxSize jobs, or a config
rebuild), a re-query counts those segments as pending again and the job reports
IN_PROGRESS forever. A normal reload never flips back like this. Could we make
the skip durable (e.g. bump loadTimeMs, or treat a missing-but-dispatched job
as complete) or at least document the caveat?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java:
##########
@@ -169,15 +169,41 @@ void deleteSegment(String segmentName)
/// `forceDownload` flag is true.
/// This operation is conducted within a failure handling framework and made
transparent to ongoing queries, because
/// the segment is in online serving state.
- void reloadSegment(String segmentName, boolean forceDownload, String
reloadJobId)
+ default void reloadSegment(String segmentName, boolean forceDownload, String
reloadJobId)
+ throws Exception {
+ reloadSegment(segmentName, forceDownload, false, reloadJobId);
+ }
+
+ /// Same as [#reloadSegment(String, boolean, String)], but when
`onlyIfNeeded` is true (and `forceDownload` is false)
+ /// the segment is skipped if its loaded copy is already up to date: same
CRC as ZK, already on the target tier, and
+ /// no preprocessing needed under the current table config and schema.
Consuming segments are always skipped in this
+ /// mode. Skipped segments are recorded in the reload job status and
reported as successful.
+ ///
+ /// This does not cover settings that only take effect when a segment is
loaded (e.g. read mode or tier storage
+ /// properties) or rebuilding derived state such as upsert metadata; a full
reload is still needed for those.
+ void reloadSegment(String segmentName, boolean forceDownload, boolean
onlyIfNeeded, String reloadJobId)
Review Comment:
This breaks external implementers of this SPI. Turning the old 3-arg methods
into `default` while adding these new *abstract* overloads means any
out-of-tree `TableDataManager` (plugged in via
`table.data.manager.provider.class`) that only implemented the 3-arg versions
will no longer compile, and an already-built one throws `AbstractMethodError`
since the server now always calls the 4-arg form.
Since `japicmp.skip=true`, CI won't catch this. Consider giving the new
overloads a `default` body that falls back to the old behavior (ignoring
`onlyIfNeeded`) and overriding them in `BaseTableDataManager` — that keeps
existing implementers working. Same applies to `InstanceDataManager`.
--
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]
