Hello Team, May I know why concurrentHashMap.remove() is not used to evict upsert metadata from memory when a segment is expired ? https://github.com/apache/pinot/blob/ca37a1e90982b2495632720645ff6a5414508afb/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java#L169 In this code, concurrentHashMap.computeIfPresent() is used to return null when the record is expired. Maintaining the expired key in the map will cause Memory overhead which can be reduced if remove() is used.

We are working on delete, that should take care of it . cc @Navina

Thanks Mayank. Do you mean this is a bug ? Is the work on Delete same as that of TTL ?

Not really a functional bug but inefficiency, because query response should be correct.

@Rahul Patwari I think this logic is the result of multiple rounds of code refactoring with attempts to share the same code via different call paths. Iiuc, the primary key index (the upsert metadata map that is in-memory) is representative of the entire partition. So, it is possible that the latest value for a primary key is not in the segment being removed. but in either a segment before or after the segment being removed.

Thanks @Navina. My question was for the case where the latest value of a primary key is in a segment that is expired. I see the condition that checks whether the primary key is in the segment that is being removed. If so, null is returned. I was expecting ConcurrentHashMap.remove(primaryKey) if the primary key is in a segment that is being removed due to retention.

@Rahul Patwari ah I see your point. I think it is a bug. If we are removed segment due to retention, there should be an explicit step to expire keys from metadata. Do you mind filing an GH issue for this? I am not fully convinced if using map.remove is the solution here since the removeSegment is used in more than one codepath.

created this ticket Navina: https://github.com/apache/pinot/issues/10769

protected void removeSegment(IndexSegment segment, MutableRoaringBitmap validDocIds) {
    assert !validDocIds.isEmpty();
    PrimaryKey primaryKey = new PrimaryKey(new Object[_primaryKeyColumns.size()]);
    PeekableIntIterator iterator = validDocIds.getIntIterator();
    try (
        UpsertUtils.PrimaryKeyReader primaryKeyReader = new UpsertUtils.PrimaryKeyReader(segment, _primaryKeyColumns)) {
      while (iterator.hasNext()) {
        primaryKeyReader.getPrimaryKey(iterator.next(), primaryKey);
        _primaryKeyToRecordLocationMap.computeIfPresent(HashUtils.hashPrimaryKey(primaryKey, _hashFunction),
            (pk, recordLocation) -> {
              if (recordLocation.getSegment() == segment) {
                return null;
              }
              return recordLocation;
            });
      }
    }
  }

CMIIW. In this code, I think the primary key entry is removed if the latest value for that primary key is in the segment that is being removed. I have tested that ConcurrentHashMap.computeIfPresent() removes the key when null is returned in the BiFunction. removeSegment() method is called when the segment is expired, right. I am not sure if all the codepaths call this method when a segment is removed.

@Jackie this seems like a bug to me. But I am unclear because removeSegment has multiple call hierarchies. Would it be safe to remove it from metadata map?

@Rahul Patwari What is the concern here? When

computeIfPresent()

returns

null

in the lambda, it is equivalent to calling

remove()

. We don't directly call

remove()

to avoid race condition: if we skip the reference check of the segment, it will remove the wrong entry if the PK is just updated to point to a new location

Understood. Thanks Jackie. Would you know if the concurrent hash map shrinks its size when entries are removed from the map ?

No. I don't see a method in

ConcurrentHashMap

that can shrink its size, so probably the only way is to re-construct a new map