Vamsi-klu commented on code in PR #19084:
URL: https://github.com/apache/pinot/pull/19084#discussion_r3725702769
##########
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:
This does compile. RoaringBitmap has a public varargs overload, public void
add(int... dat), which I verified with javap against the RoaringBitmap 1.6.16
jar, the version pinned in this branch's root pom (master has since moved to
1.6.18 on the same 1.6.x line). The method carries ACC_VARARGS and is not
deprecated. With three int arguments the only applicable overload is
add(int...), so resolution is unambiguous, and CI on this PR compiled and
passed. A similar multi-arg pattern already exists in the tests, for example
validDocIdsSnapshot2.add(0, 2, 3) in
ConcurrentMapPartitionUpsertMetadataManagerForConsistentDeletesTest, though
that variable is a MutableRoaringBitmap, which shares the same varargs
overload. That said, I will switch this line to RoaringBitmap.bitmapOf(0, 1, 2)
for consistency with the more common style in the codebase.
##########
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, agreed. tearDown() currently only deletes the temp dir, so the
mock stays in the process-global MINION_METRICS_INSTANCE. Since
MinionMetrics.register() only swaps when the current instance is NOOP, later
tests in the same JVM could not displace the leaked mock. I will capture the
previous value of the AtomicReference 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]
