Copilot commented on code in PR #17800:
URL: https://github.com/apache/pinot/pull/17800#discussion_r2879787601
##########
pinot-plugins/pinot-file-system/pinot-s3/src/test/java/org/apache/pinot/plugin/filesystem/S3ConfigTest.java:
##########
@@ -75,4 +76,16 @@ public void testLegacyMd5Plugin() {
S3Config cfg = new S3Config(pinotConfig);
Assert.assertTrue(cfg.useLegacyMd5Plugin());
}
+
+ @Test(expectedExceptions = IllegalStateException.class)
+ public void testLegacyMd5PluginWhenMd5Disabled() {
+ PinotConfiguration pinotConfig = new PinotConfiguration();
+ pinotConfig.setProperty("useLegacyMd5Plugin", "true");
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ new S3Config(pinotConfig);
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
Review Comment:
This test mutates the global `PinotMd5Mode` flag and resets it to `false`
unconditionally. To avoid leaking state into other tests (especially when the
original value was `true`), save the original
`PinotMd5Mode.isPinotMd5Disabled()` value before setting it and restore that
value in the `finally` block.
```suggestion
boolean originalPinotMd5Disabled = PinotMd5Mode.isPinotMd5Disabled();
try {
PinotMd5Mode.setPinotMd5Disabled(true);
new S3Config(pinotConfig);
} finally {
PinotMd5Mode.setPinotMd5Disabled(originalPinotMd5Disabled);
```
##########
pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java:
##########
@@ -141,6 +142,34 @@ public DataLakeFileSystemClient
getOrCreateClientWithFileSystem(DataLakeServiceC
assertTrue(sasTokenFS != null);
}
+ @Test
+ public void testChecksumEnabledWithMd5DisabledFails() {
+ PinotConfiguration pinotConfiguration = new PinotConfiguration();
+ pinotConfiguration.setProperty("authenticationType", "SAS_TOKEN");
+ pinotConfiguration.setProperty("sasToken",
"sp=rwdl&se=2025-12-31T23:59:59Z&sv=2022-11-02&sr=c&sig=test");
+ pinotConfiguration.setProperty("accountName", "testaccount");
+ pinotConfiguration.setProperty("fileSystemName", "testcontainer");
+ pinotConfiguration.setProperty("enableChecksum", "true");
+
+ ADLSGen2PinotFS adlsGen2PinotFs = new ADLSGen2PinotFS() {
+ @Override
+ public DataLakeFileSystemClient
getOrCreateClientWithFileSystem(DataLakeServiceClient serviceClient,
+ String fileSystemName) {
+ return _mockFileSystemClient;
+ }
+ };
+
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ IllegalStateException exception =
+ expectThrows(IllegalStateException.class, () ->
adlsGen2PinotFs.init(pinotConfiguration));
+ assertTrue(exception.getMessage().contains("pinot.md5.disabled"));
+ assertTrue(exception.getMessage().contains("enableChecksum"));
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
+ }
Review Comment:
This test sets the global `PinotMd5Mode` flag and then resets it to `false`
unconditionally. To keep tests isolated (and compatible with runs where the
flag is originally `true` via system property), capture the original
`PinotMd5Mode.isPinotMd5Disabled()` value and restore it in the `finally` block
instead of hard-coding `false`.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java:
##########
@@ -2025,6 +2027,81 @@ public void testValidateInvalidDedupConfigs() {
}
}
+ @Test
+ public void testValidateUpsertConfigWithMd5Disabled() {
+ Schema schema = new Schema.SchemaBuilder().setSchemaName(TABLE_NAME)
+ .addSingleValueDimension("myCol", FieldSpec.DataType.STRING)
+ .setPrimaryKeyColumns(Lists.newArrayList("myCol"))
+ .build();
+ UpsertConfig upsertConfig = new UpsertConfig(UpsertConfig.Mode.FULL);
+ upsertConfig.setHashFunction(HashFunction.MD5);
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.REALTIME).setTableName(TABLE_NAME)
+ .setUpsertConfig(upsertConfig)
+ .setRoutingConfig(
+ new RoutingConfig(null, null,
RoutingConfig.STRICT_REPLICA_GROUP_INSTANCE_SELECTOR_TYPE, false))
+ .setStreamConfigs(getStreamConfigs())
+ .build();
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ IllegalStateException exception =
+ expectThrows(IllegalStateException.class, () ->
TableConfigUtils.validateUpsertAndDedupConfig(tableConfig,
+ schema));
+
assertTrue(exception.getMessage().contains(CommonConstants.CONFIG_OF_PINOT_MD5_DISABLED));
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
+ }
Review Comment:
This test toggles the global `PinotMd5Mode` flag and resets it to `false`
unconditionally. To avoid impacting other tests when the original value was
`true` (e.g., set via system property or another test), store the original
`PinotMd5Mode.isPinotMd5Disabled()` value before changing it and restore that
value in the `finally` block.
##########
pinot-spi/src/test/java/org/apache/pinot/spi/utils/PinotMd5ModeTest.java:
##########
@@ -0,0 +1,60 @@
+/**
+ * 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.spi.utils;
+
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+
+public class PinotMd5ModeTest {
+ @Test
+ public void testSetterGetter() {
+ try {
+ PinotMd5Mode.setPinotMd5Disabled(true);
+ assertTrue(PinotMd5Mode.isPinotMd5Disabled());
+ PinotMd5Mode.setPinotMd5Disabled(false);
+ assertFalse(PinotMd5Mode.isPinotMd5Disabled());
+ } finally {
+ PinotMd5Mode.setPinotMd5Disabled(false);
Review Comment:
Tests modify the global PinotMd5Mode flag but always reset it to `false` in
the `finally` block. This can break test isolation when the original value was
`true` (e.g., when running tests with `-Dpinot.md5.disabled=true`) and can
affect other tests in the same JVM. Capture the original
`PinotMd5Mode.isPinotMd5Disabled()` value at the start of the test and restore
it in `finally` (or reset from the system property) instead of hard-coding
`false`.
```suggestion
boolean originalDisabled = PinotMd5Mode.isPinotMd5Disabled();
try {
PinotMd5Mode.setPinotMd5Disabled(true);
assertTrue(PinotMd5Mode.isPinotMd5Disabled());
PinotMd5Mode.setPinotMd5Disabled(false);
assertFalse(PinotMd5Mode.isPinotMd5Disabled());
} finally {
PinotMd5Mode.setPinotMd5Disabled(originalDisabled);
```
--
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]
