xiangfu0 commented on code in PR #19738:
URL: https://github.com/apache/pinot/pull/19738#discussion_r4290444930
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/helix/HelixHelper.java:
##########
@@ -78,9 +78,31 @@ private HelixHelper() {
public static final String BROKER_RESOURCE =
CommonConstants.Helix.BROKER_RESOURCE_INSTANCE;
+ /// Returns a deep copy of the IdealState with the same content as cloning
it through a serialization round trip of
+ /// its ZNRecord: id, simple fields, map fields, list fields and raw
payload, with the same container types and
+ /// iteration order. Like the round trip, the ZK stat fields (e.g. version)
are not copied. Unlike the round trip, the
+ /// serializer's write policies (list field bound, size limit) are not
applied; they still apply when the IdealState
+ /// is written.
+ ///
+ /// Copying the fields directly avoids serializing (and compressing) the
whole ZNRecord and parsing it back, which
+ /// takes seconds for an IdealState with hundreds of thousands of segments
and widens the window for version
+ /// conflicts when updating it.
public static IdealState cloneIdealState(IdealState idealState) {
- return new IdealState(
- (ZNRecord)
ZN_RECORD_SERIALIZER.deserialize(ZN_RECORD_SERIALIZER.serialize(idealState.getRecord())));
+ ZNRecord record = idealState.getRecord();
+ IdealState copy = new IdealState(record.getId());
+ ZNRecord copyRecord = copy.getRecord();
+ copyRecord.setSimpleFields(new TreeMap<>(record.getSimpleFields()));
+ TreeMap<String, Map<String, String>> mapFields = new
TreeMap<>(record.getMapFields());
+ mapFields.replaceAll((key, value) -> value != null ? new
LinkedHashMap<>(value) : null);
+ copyRecord.setMapFields(mapFields);
+ TreeMap<String, List<String>> listFields = new
TreeMap<>(record.getListFields());
+ listFields.replaceAll((key, value) -> value != null ? new
ArrayList<>(value) : null);
+ copyRecord.setListFields(listFields);
+ byte[] rawPayload = record.getRawPayload();
+ if (rawPayload != null) {
+ copyRecord.setRawPayload(rawPayload.clone());
+ }
+ return copy;
Review Comment:
Nice — the deep copy looks correct here. The outer `new TreeMap<>(...)` plus
`replaceAll` with fresh `LinkedHashMap`/`ArrayList` gives genuinely independent
maps and lists, so no mutable state is shared back to the source record. It
also matches what the old round trip produced (sorted TreeMap outer maps,
LinkedHashMap inner maps, ArrayList lists), which the test confirms.
--
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]
