Vamsi-klu commented on code in PR #19084:
URL: https://github.com/apache/pinot/pull/19084#discussion_r3755546049
##########
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:
Switched to `RoaringBitmap.bitmapOf(0, 1, 2)` in `f60b376`. For the record
the varargs `add(int...)` overload does compile and CI was green on it, so this
is a readability change rather than a fix.
##########
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:
Good catch, that would have leaked the mock into every later test in the
same JVM. Fixed in `f60b376`: `setUp` captures the previous instance from the
same `AtomicReference` before installing the mock, and `tearDown` restores it
with `alwaysRun = true` so a failing assertion cannot skip the restore. All the
reflection lives in one helper now, so a future JDK encapsulation change breaks
in exactly one place.
--
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]
