Copilot commented on code in PR #19084:
URL: https://github.com/apache/pinot/pull/19084#discussion_r3669318308
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskExecutorTest.java:
##########
@@ -64,4 +127,282 @@ public void testGetServers() {
() -> MinionTaskUtils.getServers(SEGMENT_NAME, REALTIME_TABLE_NAME,
helixManager.getClusterManagmentTool(),
helixManager.getClusterName()));
}
+
+ @Test
+ public void testValidateDeepStoreCrcMatch() {
+ TestableExecutor executor = newExecutor();
+ executor.validateDeepStoreCrc(REALTIME_TABLE_NAME, SEGMENT_NAME,
EXPECTED_CRC, EXPECTED_CRC, DATA_CRC, false);
+ verify(_minionMetrics, never()).addMeteredTableValue(anyString(),
eq(MinionMeter.CRC_MISMATCH_DEEPSTORE),
+ anyLong());
+ }
+
+ @Test
+ public void testValidateDeepStoreCrcMismatchThrowsAndMeters() {
+ TestableExecutor executor = newExecutor();
+ executor._zkDataCrc = -1;
+ try {
+ executor.validateDeepStoreCrc(REALTIME_TABLE_NAME, SEGMENT_NAME,
EXPECTED_CRC, "9999", DATA_CRC, false);
+ Assert.fail("expected IllegalStateException");
+ } catch (IllegalStateException e) {
+ Assert.assertTrue(e.getMessage().contains("Crc mismatched"));
+ }
+ verify(_minionMetrics).addMeteredTableValue(REALTIME_TABLE_NAME,
MinionMeter.CRC_MISMATCH_DEEPSTORE, 1L);
+ }
+
+ @Test
+ public void testValidateDeepStoreCrcDataCrcFallback() {
+ // Segment CRCs differ but both sides report the same data CRC → match
(index-only drift).
+ TestableExecutor executor = newExecutor();
+ executor._zkDataCrc = Long.parseLong(DATA_CRC);
+ executor.validateDeepStoreCrc(REALTIME_TABLE_NAME, SEGMENT_NAME,
EXPECTED_CRC, "9999", DATA_CRC, false);
+ verify(_minionMetrics, never()).addMeteredTableValue(anyString(),
eq(MinionMeter.CRC_MISMATCH_DEEPSTORE),
+ anyLong());
+ }
+
+ @Test
+ public void testValidateDeepStoreCrcIgnoreMismatch() {
+ TestableExecutor executor = newExecutor();
+ executor._zkDataCrc = -1;
+ executor.validateDeepStoreCrc(REALTIME_TABLE_NAME, SEGMENT_NAME,
EXPECTED_CRC, "9999", DATA_CRC, true);
+ verify(_minionMetrics, never()).addMeteredTableValue(anyString(),
eq(MinionMeter.CRC_MISMATCH_DEEPSTORE),
+ anyLong());
+ }
+
+ @Test
+ public void testFetchValidDocIdsRetrySucceedsAfterTransientCrcMismatch()
+ throws Exception {
+ TestableExecutor executor = newExecutor();
+ executor._validDocIdsFetchMaxAttempts = 3;
+ executor._validDocIdsFetchRetryDelayMs = 0L;
+ RoaringBitmap bitmap = new RoaringBitmap();
+ bitmap.add(0, 1, 2);
Review Comment:
`RoaringBitmap#add(0, 1, 2)` is not a supported overload in the
RoaringBitmap API used elsewhere in Pinot (usage is consistently `add(int)` or
`bitmapOf(int...)`). This will likely fail compilation; add the values
individually (or via a range) instead.
##########
pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskExecutorTest.java:
##########
@@ -18,24 +18,87 @@
*/
package org.apache.pinot.plugin.minion.tasks.upsertcompaction;
+import java.io.File;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.commons.io.FileUtils;
import org.apache.helix.HelixAdmin;
import org.apache.helix.HelixManager;
import org.apache.helix.model.ExternalView;
+import org.apache.pinot.common.metrics.MinionMeter;
+import org.apache.pinot.common.metrics.MinionMetrics;
+import org.apache.pinot.core.common.MinionConstants;
+import org.apache.pinot.core.common.MinionConstants.UpsertCompactionTask;
+import org.apache.pinot.core.minion.PinotTaskConfig;
import org.apache.pinot.minion.MinionContext;
+import org.apache.pinot.minion.event.MinionEventObserver;
+import org.apache.pinot.plugin.minion.tasks.MinionTaskTestUtils;
import org.apache.pinot.plugin.minion.tasks.MinionTaskUtils;
+import org.apache.pinot.plugin.minion.tasks.SegmentConversionResult;
+import org.apache.pinot.segment.spi.index.metadata.SegmentMetadataImpl;
+import org.apache.pinot.spi.config.table.TableConfig;
+import org.apache.pinot.spi.config.table.TableTaskConfig;
+import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.config.table.UpsertConfig;
import
org.apache.pinot.spi.utils.CommonConstants.Helix.StateModel.SegmentStateModel;
+import org.apache.pinot.spi.utils.Enablement;
+import org.apache.pinot.spi.utils.builder.TableConfigBuilder;
+import org.mockito.MockedConstruction;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
+import org.roaringbitmap.RoaringBitmap;
import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
public class UpsertCompactionTaskExecutorTest {
private static final String REALTIME_TABLE_NAME = "testTable_REALTIME";
private static final String SEGMENT_NAME = "testSegment";
private static final String CLUSTER_NAME = "testCluster";
+ private static final String TASK_TYPE = UpsertCompactionTask.TASK_TYPE;
+ private static final String EXPECTED_CRC = "1000";
+ private static final String DATA_CRC = "5000";
+
+ private MinionMetrics _minionMetrics;
+ private MinionEventObserver _eventObserver;
+ private File _tempDir;
+
+ @BeforeMethod
+ public void setUp()
+ throws Exception {
+ _minionMetrics = mock(MinionMetrics.class);
+ // Force the process-global singleton so BaseTaskExecutor picks up the
mock.
+ // register() only wins when current is NOOP; tests may run after other
classes registered.
+ java.lang.reflect.Field field =
MinionMetrics.class.getDeclaredField("MINION_METRICS_INSTANCE");
+ field.setAccessible(true);
+ @SuppressWarnings("unchecked")
+ java.util.concurrent.atomic.AtomicReference<MinionMetrics> ref =
+ (java.util.concurrent.atomic.AtomicReference<MinionMetrics>)
field.get(null);
+ ref.set(_minionMetrics);
+
+ _eventObserver = MinionTaskTestUtils.getMinionProgressObserver();
+ _tempDir = new File(FileUtils.getTempDirectory(),
"UpsertCompactionTaskExecutorTest-" + System.nanoTime());
+ Assert.assertTrue(_tempDir.mkdirs());
+ }
+
+ @AfterMethod
+ public void tearDown()
+ throws Exception {
+ FileUtils.deleteDirectory(_tempDir);
+ }
Review Comment:
This test mutates the process-global `MinionMetrics.MINION_METRICS_INSTANCE`
but never restores it, which can leak state into unrelated tests and cause
order-dependent failures. Capture the previous instance in `setUp()` and
restore it in `tearDown()`.
--
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]
