the obvious way is to move to a HDFS based filesystem of course. But we could also do the CRC comparison upfront and skip the metadata update ? Just trying to understand the implication of this change

https://github.com/apache/incubator-pinot/blob/cf980cbe564dda983fccf2bf7eabf877a7721cf8/pinot-controller/src/main/java/org/apache/pinot/controller/api/upload/ZKOperator.java#L153

if (newCrc == existingCrc) {
        <http://LOGGER.info|LOGGER.info>(
            "New segment crc '{}' is the same as existing segment crc for segment '{}'. Updating ZK metadata without refreshing the segment.",
            newCrc, segmentName);
        // NOTE: even though we don't need to refresh the segment, we should still update creation time and refresh time
        // (creation time is not included in the crc)
        existingSegmentZKMetadata.setCreationTime(segmentMetadata.getIndexCreationTime());
        existingSegmentZKMetadata.setRefreshTime(System.currentTimeMillis());
        if (!_pinotHelixResourceManager.updateZkMetadata(offlineTableName, existingSegmentZKMetadata)) {
          throw new RuntimeException(
              "Failed to update ZK metadata for segment: " + segmentName + " of table: " + offlineTableName);
        }

we are updating the segment metadata in order to update the refresh time

Why?

the above code writes existingSegmentZKMetadata to zk. so it could contain more than refreshed time: in our setup, it also contains a possibly different controller's download url. And if that controller uses local disk storage and does not have the segment, it could means download failures.

it looks like we should reset the download url to the existing znRecord's download url. I.e.,

existingSegmentZKMetadata.setDownloadUrl(znRecord.getSimpleField(CommonConstants.Segment.Offline.DOWNLOAD_URL));

Still trying to understand why we are changing the time? Is this for GDPR?

I believe this is for GDPR. We rely on segment refresh time to see if the segment needs to be checked for purge

I see. But we're also modifying the zkDownloadUri

Or, is it that we just flagged the segment as purged. ,I am not sure

need to check minion coe

code

maybe we can skip that ? (if crc is the same)

hmm. i dont see reference to refresh time in minion, only to crc. @User do you know why we did PR 4112?

@User Refresh time is used to determine whether a segment is subject to be purged

@User The issue you got is not caused by setting new creation time/refresh time but by setting new download URI right?

yes

correct

It's really a transient issue (once we move to HDFS based filesystem we should be good to go). But if its easy to fix - I'd love to do it

@User We upload the metadata because we lock the segment by updating metadata when pushing segment, and we need to unlock it, not because updating creation/refresh time

The issue here is download URI should be modified only if segment is moved to the permanent directory

not exactly, even if its moved to the permanent directory - this can still happen

because we don't have a shared filesystem between the controllers today - the second controller overrides the download URI to its local path

which obviously doesn't have that segment

to clarify - this is very rare (but does happen every now and then)

https://github.com/apache/incubator-pinot/pull/3234 This PR changed the updating download uri logic. Will submit a fix soon

awesome ! thank you

on a related note, it seems we're moving away from using the original download URI (eg: on HDFS) to using Pinot's "filesystem" (either local disk/NFS/HDFS). Is this because we think the original download URI is not under Pinot's control and will not be valid after some time ?