klsince commented on code in PR #13107:
URL: https://github.com/apache/pinot/pull/13107#discussion_r1619257347
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -158,6 +158,44 @@ protected void addOrReplaceSegment(ImmutableSegmentImpl
segment, ThreadSafeMutab
}
}
+ /**
+ * <li> When the replacing segment and current segment are of {@link
LLCSegmentName} then the PK should resolve to
+ * row in segment with higher sequence id.
+ * <li> When the replacing segment and current segment are of {@link
UploadedRealtimeSegmentName} then the PK
+ * should resolve to row in segment with higher sequence id, creation time.
+ * <li> When either is of type {@link UploadedRealtimeSegmentName} then
resolve on creation time, if same(rare
+ * scenario) then give preference to uploaded time
+ *
+ * @param segmentName replacing segment
+ * @param currentSegmentName current segment having the record for the given
primary key
+ * @param segmentCreationTimeMs creation time of replacing segment
+ * @param currentSegmentCreationTimeMs creation time of current segment
+ * @return true if the record in replacing segment should replace the record
in current segment
+ */
+ private boolean shouldReplaceOnComparisonTie(String segmentName, String
currentSegmentName,
+ long segmentCreationTimeMs, long currentSegmentCreationTimeMs) {
+
+ if (LLCSegmentName.isLLCSegment(segmentName) &&
LLCSegmentName.isLLCSegment(currentSegmentName)
Review Comment:
I think the comparison of seqIds shouldn't be part of the if-check, as
that'd miss a case when both are LLC but seqIds are not `a > b`.
```
if (LLC && LLC) {
return seqId_a > seqId_b;
}
```
And we can do LLC.of() and check nulls to save some parsing cost, as both
isLLCSegemnt() and getSequenceNumber() does parsing inside. Same for the checks
on uploaded names below, we can do Uploaded.of() and check nulls to save some
cost of regex evaluation.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/UploadedRealtimeSegmentName.java:
##########
@@ -0,0 +1,140 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
+import java.util.Objects;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import javax.annotation.Nullable;
+
+
+/**
+ * Class to represent segment names like:
uploaded_{tableName}_{partitionId}_{sequenceId}_{creationTime}
+ *
+ * This naming convention is adopted to represent a batch generated segment
uploaded to a realtime table. The naming
+ * convention has been kept different from {@LLCSegmentName} to differentiate
between batch generated segments and
+ * low level consumer segments.
+ */
+public class UploadedRealtimeSegmentName implements
Comparable<UploadedRealtimeSegmentName> {
+
+ public static final String UPLOADED_REALTIME_SEGMENT_NAME_REGEX =
"^uploaded_(.+)_(\\d+)_(\\d+)_(\\d+)$";
+
+ private static final Pattern NAME_PATTERN =
Pattern.compile(UPLOADED_REALTIME_SEGMENT_NAME_REGEX);
+ private static final String UPLOADED_PREFIX = "uploaded";
+ private static final String SEPARATOR = "_";
+ private final String _tableName;
+ private final int _partitionId;
+ private final int _sequenceId;
+ private final long _creationTime;
+ private final String _segmentName;
+
+ public UploadedRealtimeSegmentName(String segmentName) {
+
+ Matcher matcher = NAME_PATTERN.matcher(segmentName);
+
+ if (matcher.find()) {
+ _tableName = matcher.group(1);
+ _partitionId = Integer.parseInt(matcher.group(2));
+ _sequenceId = Integer.parseInt(matcher.group(3));
+ _creationTime = Long.parseLong(matcher.group(4));
+
+ _segmentName = segmentName;
+ } else {
+ throw new IllegalArgumentException("Invalid uploaded realtime segment
name: " + segmentName);
+ }
+ }
+
+ public UploadedRealtimeSegmentName(String tableName, int partitionId, int
sequenceId, long creationTime) {
+ _tableName = tableName;
+ _partitionId = partitionId;
+ _sequenceId = sequenceId;
+ _creationTime = creationTime;
Review Comment:
should we also adopt the time format as in LLC segment name?
```
_creationTime = DATE_FORMATTER.print(msSinceEpoch);
```
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/UploadedRealtimeSegmentName.java:
##########
@@ -0,0 +1,131 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils;
+
+import com.google.common.base.Preconditions;
+import java.util.Objects;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+
+/**
+ * Class to represent segment names like:
uploaded_{tableName}_{partitionId}_{sequenceId}_{creationTime}
+ *
+ * This naming convention is adopted to represent a batch generated segment
uploaded to a realtime table. The naming
+ * convention has been kept different from {@LLCSegmentName} to differentiate
between batch generated segments and
+ * low level consumer segments.
+ */
+public class UploadedRealtimeSegmentName implements
Comparable<UploadedRealtimeSegmentName> {
+
+ public static final String UPLOADED_REALTIME_SEGMENT_NAME_REGEX =
"^uploaded_(.+)_(\\d+)_(\\d+)_(\\d+)$";
Review Comment:
I'd be a bit concerned on the cost of evaluating regex, as the method is
used on the upsert comparison logic path as well. In worst case, the regex may
be evaluated for every doc ingested (as the upload segment might be the winner
all the time). We may do a micro benchmark on the two ways: regex vs. split
(the code changes you proposed above, but better use StringUtils.split() as
String.split() does regex internally as well...)
--
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
