jackjlli commented on code in PR #15585:
URL: https://github.com/apache/pinot/pull/15585#discussion_r2054775407
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java:
##########
@@ -214,8 +217,19 @@ public Response downloadSegment(
segmentFile =
org.apache.pinot.common.utils.FileUtils.concatAndValidateFile(tableDir,
segmentName + "-" + UUID.randomUUID(),
"Invalid segment name: %s", segmentName);
-
+ long deepStoreDownloadStartTimeMs = System.currentTimeMillis();
pinotFS.copyToLocalFile(remoteSegmentFileURI, segmentFile);
+ long deepStoreDownloadDurationMs = System.currentTimeMillis() -
deepStoreDownloadStartTimeMs;
+ _controllerMetrics.addTimedTableValue(tableName,
ControllerTimer.SEGMENT_DEEP_STORE_DOWNLOAD_TIME_MS,
Review Comment:
This line and the below line seem to be very similar though, is it that one
for table level and one for server level? Why do we need to have two separate
metrics? Can we just emit 1 metric for 2 purposes?
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -434,9 +438,22 @@ protected void
replaceSegmentIfCrcMismatch(SegmentDataManager segmentDataManager
@Override
public void downloadAndLoadSegment(SegmentZKMetadata zkMetadata,
IndexLoadingConfig indexLoadingConfig)
throws Exception {
+ long downloadStartTimeMs = System.currentTimeMillis();
String segmentName = zkMetadata.getSegmentName();
_logger.info("Downloading and loading segment: {}", segmentName);
+ // Download the segment
File indexDir = downloadSegment(zkMetadata);
+ long totalDownloadDurationMs = System.currentTimeMillis() -
downloadStartTimeMs;
+ _serverMetrics.addTimedValue(_rawTableName,
ServerTimer.SEGMENT_TOTAL_DOWNLOAD_TIME_MS,
+ totalDownloadDurationMs, TimeUnit.MILLISECONDS);
+ _serverMetrics.addTimedValue(ServerTimer.SEGMENT_TOTAL_DOWNLOAD_TIME_MS,
totalDownloadDurationMs,
+ TimeUnit.MILLISECONDS);
+ // Load the segment
+ addSegment(ImmutableSegmentLoader.load(indexDir, indexLoadingConfig,
_segmentOperationsThrottler));
+ long loadDurationMs = System.currentTimeMillis() - downloadStartTimeMs -
totalDownloadDurationMs;
Review Comment:
instead of using two `-` operators here, can we use one more variable to
store the `downloadEndTimeMs` and just it the new variable here?
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/Server2ControllerSegmentUploader.java:
##########
@@ -91,6 +92,8 @@ private SegmentCompletionProtocol.Response
uploadSegmentToController(File segmen
SegmentCompletionProtocol.Response response;
long startTime = System.currentTimeMillis();
try {
+ SegmentCompletionUtils.updateActiveUploadSegmentCount(_serverMetrics,
_rawTableName,
Review Comment:
IIUC this `Server2ControllerSegmentUploader` is intialized per segment (you
can see from the constructor of this class that the segment name is passed into
this class). So the segment active count here for each server should only be
either 0 or 1, and this doesn't add up for the whole table per server. So just
want to double click on this one here.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/PinotFSSegmentUploader.java:
##########
@@ -51,11 +51,13 @@ public class PinotFSSegmentUploader implements
SegmentUploader {
private final ExecutorService _executorService =
Executors.newCachedThreadPool();
private final int _timeoutInMs;
private final ServerMetrics _serverMetrics;
+ private final AtomicInteger _segmentActiveCount;
Review Comment:
nit: can we rename it to make it more specific to upload? The current used
name will be quite confusing.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
