Segment reload in upsert table took too much time and caused disruption

[chenboat]

Reloading a segment in a Pinot upsert table needs around 20 seconds during a table segment reload (see timeline below). In a server with hundreds of segments, the reload of all segments could take hours and causes disruption.

As a comparison, if we turn off the upsert feature for the same realtime table, a reload of segment of the same size needs just 0.2 second which is 50 times faster.

@Jackie-Jiang @yupeng9 @icefury71

[chenboat]

As can be seen from the event sequence below, the follow call Removing upsert metadata for segment: took 20s to finish and is the main culprit of the slow reload.

In segment reload, an existing segment A is replicated to another copy A' to continue serving. When the segment finishes reloading, A' is destroyed. A upsert table metadata manager needs to delete all the references to the old segment in the upsert metadata table _primaryKeyToRecordLocationMap. This step right now takes too long for finish and should be optimized. One interesting note is that during segment reload, in fact none of the segment location is changed logically and they can be left untouched.

Event	timestamp
Closed segment: rta_ads_metrics__3__72__20210611T1841Z of table: rta_ads_me...	2021-10-12T22:01:04.719+00:00
Reloaded segment: rta_ads_metrics__3__72__20210611T1841Z in table: rta_ads_...	2021-10-12T22:01:04.719+00:00
Removing upsert metadata for segment: rta_ads_metrics__3__72__20210611T1841...	2021-10-12T22:00:42.629+00:00
Closing segment: rta_ads_metrics__3__72__20210611T1841Z of table: rta_ads_m...	2021-10-12T22:00:42.628+00:00
Adding immutable segment: rta_ads_metrics__3__72__20210611T1841Z to table: ...	2021-10-12T22:00:42.628+00:00
Replaced immutable segment: rta_ads_metrics__3__72__20210611T1841Z of table...	2021-10-12T22:00:42.628+00:00
Trying to destroy segment : rta_ads_metrics__3__72__20210611T1841Z	2021-10-12T22:00:42.628+00:00
Adding upsert metadata for segment: rta_ads_metrics__3__72__20210611T1841Z	2021-10-12T22:00:41.431+00:00
Successfully loaded segment rta_ads_metrics__3__72__20210611T1841Z with con...	2021-10-12T22:00:41.431+00:00
Found inverted index for segment: rta_ads_metrics__3__72__20210611T1841Z, c...	2021-10-12T22:00:41.426+00:00
Found inverted index for segment: rta_ads_metrics__3__72__20210611T1841Z, c...	2021-10-12T22:00:41.426+00:00
Found inverted index for segment: rta_ads_metrics__3__72__20210611T1841Z, c...	2021-10-12T22:00:41.426+00:00
Reloading segment: rta_ads_metrics__3__72__20210611T1841Z in table: rta_ads...	2021-10-12T22:00:41.378+00:00

[Jackie-Jiang]

@chenboat Great findings! Can you please also check the size of the _primaryKeyToRecordLocationMap? Current algorithm loops over all the entries within the map, and could be slow when the map is huge.

[yupeng9]

That's very good insights, Thanks @chenboat. The map size is about 1.6 billion for 8 partitions. So each partition and the metadata manager has about 200million per map. I wonder if we can make the deletion asynchronous and does not block the restart? @Jackie-Jiang

[Jackie-Jiang]

@yupeng9 I feel we have to delete all the entries before the segment is loaded again to ensure correctness. One thing we can optimize is that we can remove the map entries using the segment validDocIds if the map size is larger than the validDocIds

[yupeng9]

Could you elaborate on this? Also, I feel 200million entries taking 20s is a bit long to me. @richardstartin any insight?

[Jackie-Jiang]

From the validDocIds of a segment, we know the records that are valid within the map, and only remove these primary keys instead of scanning over every entry within the map

[yupeng9]

I see. Is there an efficient implementation of this? Overlay the bitmap on the map

[richardstartin]

Can we profile it? e.g. install async-profiler from here and set -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,file=cpu.html then run lots of segment reloads? This should cut out some guesswork.

[yupeng9]

High-level question: why do we need to remove segment during reload? I feel we can simply load segments one by one without removing any segment, which can skip this expensive step shown in the log? @Jackie-Jiang @chenboat

[chenboat]

@yupeng9 The segment removal happens in the following step during adding a segment:

pinot/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

Line 157 in c9e36dd

releaseSegment(oldSegmentManager);

In segment reload, a replica of a segment is in fact added using the above step.

[yupeng9]

okay, I think the issue could be on the iteration of the entire map

pinot/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/PartitionUpsertMetadataManager.java

Line 243 in 7e9ca6a

_primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {

 _primaryKeyToRecordLocationMap.forEach((primaryKey, recordLocation) -> {
        if (recordLocation.getSegment() == segment) {
          // Check and remove to prevent removing the key that is just updated
          _primaryKeyToRecordLocationMap.remove(primaryKey, recordLocation);
        }
      });

Perhaps we could build an index of segment-> PKs, or change _primaryKeyToRecordLocationMap to a sorted map?

[chenboat]

Here is my followup investigation:
(1) There is no unit test on the following method for a normal Pinot table let alone an upsert table. We should add unit tests to verify the behaviors of both types of tables:

pinot/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

Line 195 in c9e36dd

public void reloadSegment(String tableNameWithType, String segmentName, boolean forceDownload)

(2) Similarly the addSegment() method in RealtimeTableDataManager is not unit-tested in the case of upsert table segment reload:

pinot/pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java

Lines 358 to 363 in a5f3dc5

	public void addSegment(ImmutableSegment immutableSegment) {
	if (isUpsertEnabled()) {
	handleUpsert((ImmutableSegmentImpl) immutableSegment);
	}
	super.addSegment(immutableSegment);
	}

For an upsert table segment reload, today's impl first adds the segment upsert metadata (line 359-360) and then removes the upsert metadata (line 362) from the same segment copy. This sounds a redundant sequence of operations. I can not understand if the final metadata hashmap state for upsert table is correct. A unit test should verify that and lead to possible optimization here.

[hpvd]

Is this is issue solved with merged Upsert enhancements or by any other of the upsert changes from the last 2 years?
Or are there open things to be addressed?