This is an automated email from the ASF dual-hosted git repository.
xiangfu0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 659422da244 Address review feedback from the deprecation-removal PRs
(#19148)
659422da244 is described below
commit 659422da244bea3d2e7f6dd51ea2b661b2a4cfca
Author: Xiang Fu <[email protected]>
AuthorDate: Sun Aug 2 18:27:14 2026 -0700
Address review feedback from the deprecation-removal PRs (#19148)
Follow-ups to the review comments on #19139 and #19142. No API is removed.
- Drop the now-redundant (List<IndexType>) null casts at FieldConfig call
sites.
They existed only to disambiguate the singular-IndexType 5-arg
constructor from
the List one; with the former removed in #19139, bare null resolves
unambiguously. Also drops the java.util.List import in
AvgAggregationFunctionTest,
unused once the casts are gone, and collapses the call sites that no
longer need
to wrap.
- GET /tables/livebrokers mapped every exception to 404 NOT_FOUND while
declaring
only 200 and 500. getTableToLiveBrokersMapping filters unknown tables
rather than
reporting them, so nothing it throws is a lookup miss: a missing broker
ExternalView surfaced as IllegalStateException and was reported to
clients as
'not found', and the documented 500 was unreachable. Map to
INTERNAL_SERVER_ERROR
with a stable message, keeping the cause for the logs.
- MaterializedViewTaskExecutorFactory fell back to an empty
PinotConfiguration when
_minionConf was null, which would silently build a plaintext gRPC client
with
default limits instead of the configured TLS one. With the deprecated
1-arg init
removed, that branch is dead. Replaced with Preconditions checks covering
both
_minionConf and _zkMetadataManager, at the top of create() so they run on
every
call rather than only the first.
---
.../controller/api/resources/PinotTableInstances.java | 6 +++++-
.../function/AvgAggregationFunctionTest.java | 10 +++-------
.../MaterializedViewTaskExecutorFactory.java | 18 +++++++++++-------
.../local/utils/IndexCombinationValidationTest.java | 14 ++++++--------
.../spi/config/table/OpenStructIndexConfigTest.java | 9 +++------
5 files changed, 28 insertions(+), 29 deletions(-)
diff --git
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
index 1d2c9fb2aff..51b9dd2e61b 100644
---
a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
+++
b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
@@ -172,7 +172,11 @@ public class PinotTableInstances {
try {
return
_pinotHelixResourceManager.getTableToLiveBrokersMapping(headers.getHeaderString(DATABASE),
tables);
} catch (Exception e) {
- throw new ControllerApplicationException(LOGGER, e.getMessage(),
Response.Status.NOT_FOUND);
+ // Unknown tables are filtered out rather than reported, so anything
thrown here (e.g. a missing broker
+ // ExternalView) is a server-side failure, not a lookup miss. Use a
stable message since the cause may
+ // carry none, and let the attached cause supply the detail in the logs.
+ throw new ControllerApplicationException(LOGGER, "Failed to get table to
live brokers mapping",
+ Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
diff --git
a/pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java
b/pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java
index 3e3dbc4faf5..9302073597f 100644
---
a/pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java
+++
b/pinot-core/src/test/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java
@@ -18,7 +18,6 @@
*/
package org.apache.pinot.core.query.aggregation.function;
-import java.util.List;
import org.apache.pinot.queries.FluentQueryTest;
import org.apache.pinot.spi.config.table.FieldConfig;
import org.apache.pinot.spi.config.table.TableType;
@@ -196,8 +195,7 @@ public class AvgAggregationFunctionTest extends
AbstractAggregationFunctionTest
.build(),
new TableConfigBuilder(TableType.OFFLINE)
.setTableName("testTable")
- .addFieldConfig(
- new FieldConfig("key", encoding,
(List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
+ .addFieldConfig(new FieldConfig("key", encoding, null,
PASS_THROUGH, null))
.build())
.onFirstInstance(new Object[]{7, 1}, new Object[]{6, 2}, new
Object[]{5, 3}, new Object[]{4, 4})
.andOnSecondInstance(new Object[]{7, 1}, new Object[]{6, 2}, new
Object[]{5, 3}, new Object[]{4, 4})
@@ -228,10 +226,8 @@ public class AvgAggregationFunctionTest extends
AbstractAggregationFunctionTest
.build(),
new TableConfigBuilder(TableType.OFFLINE)
.setTableName("testTable")
- .addFieldConfig(
- new FieldConfig("key1", encoding,
(List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
- .addFieldConfig(
- new FieldConfig("key2", encoding,
(List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
+ .addFieldConfig(new FieldConfig("key1", encoding, null,
PASS_THROUGH, null))
+ .addFieldConfig(new FieldConfig("key2", encoding, null,
PASS_THROUGH, null))
.build())
.onFirstInstance(new Object[]{7, 1}, new Object[]{6, 2}, new
Object[]{5, 3}, new Object[]{4, 4})
.andOnSecondInstance(new Object[]{7, 1}, new Object[]{6, 2}, new
Object[]{5, 3}, new Object[]{4, 4})
diff --git
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java
index bf9a5a1a1e0..483d4546d30 100644
---
a/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java
+++
b/pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java
@@ -18,6 +18,7 @@
*/
package org.apache.pinot.plugin.minion.tasks.materializedview;
+import com.google.common.base.Preconditions;
import org.apache.pinot.common.config.GrpcConfig;
import
org.apache.pinot.materializedview.executor.GrpcMaterializedViewQueryExecutor;
import
org.apache.pinot.materializedview.executor.MaterializedViewQueryExecutor;
@@ -59,18 +60,21 @@ public class MaterializedViewTaskExecutorFactory implements
PinotTaskExecutorFac
@Override
public PinotTaskExecutor create() {
+ // Validated on every call rather than only on the first: without a
MinionConf we would silently build a
+ // plaintext gRPC client with default limits instead of the configured
one, and without a metadata manager
+ // we would hand a null straight to the executor.
+ Preconditions.checkState(_zkMetadataManager != null,
+ "MinionTaskZkMetadataManager is not set; init(zkMetadataManager,
minionConf) must be called before create()");
+ Preconditions.checkState(_minionConf != null,
+ "MinionConf is not set; init(zkMetadataManager, minionConf) must be
called before create()");
if (_queryExecutor == null) {
synchronized (this) {
if (_queryExecutor == null) {
// Build the gRPC client config from the minion's own configuration,
scoped to the
// MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX prefix.
This is how operators
- // enable TLS, raise the max inbound message size for large MV
result sets, and tune
- // keepalive. Falling back to an empty configuration (no TLS,
defaults) when no
- // MinionConf was provided — fine for local tests but production
deployments should
- // initialize the factory with a MinionConf.
- PinotConfiguration grpcClientConfig = _minionConf != null
- ?
_minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX)
- : new PinotConfiguration();
+ // enable TLS, raise the max inbound message size for large MV
result sets, and tune keepalive.
+ PinotConfiguration grpcClientConfig =
+
_minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX);
_queryExecutor = new GrpcMaterializedViewQueryExecutor(
MinionContext.getInstance().getHelixManager(),
new GrpcConfig(grpcClientConfig));
diff --git
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java
index 50f6aa7b784..b2fb2c5b5cf 100644
---
a/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java
+++
b/pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java
@@ -458,7 +458,7 @@ public class IndexCombinationValidationTest {
@Test
public void testRawWithLz4CodecPasses() {
- FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW,
(List<IndexType>) null, CompressionCodec.LZ4, null);
+ FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null,
CompressionCodec.LZ4, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
@@ -468,7 +468,7 @@ public class IndexCombinationValidationTest {
@Test
public void testRawWithSnappyCodecPasses() {
- FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW,
(List<IndexType>) null, CompressionCodec.SNAPPY, null);
+ FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null,
CompressionCodec.SNAPPY, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
@@ -478,8 +478,7 @@ public class IndexCombinationValidationTest {
@Test
public void testRawWithZstdCodecPasses() {
- FieldConfig fc =
- new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null,
CompressionCodec.ZSTANDARD, null);
+ FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null,
CompressionCodec.ZSTANDARD, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
@@ -490,7 +489,7 @@ public class IndexCombinationValidationTest {
@Test
public void testRawWithClpCodecStringColumnPasses() {
// CLP codecs are valid for raw STRING columns
- FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW,
(List<IndexType>) null, CompressionCodec.CLP, null);
+ FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null,
CompressionCodec.CLP, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
@@ -501,7 +500,7 @@ public class IndexCombinationValidationTest {
@Test
public void testRawWithClpCodecNonStringColumnFails() {
// CLP is only valid on STRING stored type
- FieldConfig fc = new FieldConfig(INT_COL, EncodingType.RAW,
(List<IndexType>) null, CompressionCodec.CLP, null);
+ FieldConfig fc = new FieldConfig(INT_COL, EncodingType.RAW, null,
CompressionCodec.CLP, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(INT_COL))
.setFieldConfigList(List.of(fc))
@@ -512,8 +511,7 @@ public class IndexCombinationValidationTest {
@Test
public void testDeltaDeltaCodecNonNumericColumnFails() {
// DELTADELTA only valid on INT/LONG columns
- FieldConfig fc =
- new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null,
CompressionCodec.DELTADELTA, null);
+ FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null,
CompressionCodec.DELTADELTA, null);
TableConfig tc = new
TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
diff --git
a/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java
b/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java
index 43a71a4dc2b..908dc725721 100644
---
a/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java
+++
b/pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java
@@ -54,8 +54,7 @@ public class OpenStructIndexConfigTest {
@Test
public void testNoDictionaryKeys() {
- FieldConfig rawKey =
- new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW,
(List<FieldConfig.IndexType>) null, null, null);
+ FieldConfig rawKey = new FieldConfig("raw_payload",
FieldConfig.EncodingType.RAW, null, null, null);
OpenStructIndexConfig config = new OpenStructIndexConfig(false, null,
1000, null, 0.5, List.of(rawKey));
assertFalse(config.shouldUseDictionaryForKey("raw_payload"));
// Unconfigured key falls back to built-in default (DICTIONARY).
@@ -125,10 +124,8 @@ public class OpenStructIndexConfigTest {
@Test
public void testShouldUseDictionaryForKeyHardOverride() {
- FieldConfig blob =
- new FieldConfig("blob", FieldConfig.EncodingType.RAW,
(List<FieldConfig.IndexType>) null, null, null);
- FieldConfig rawPayload =
- new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW,
(List<FieldConfig.IndexType>) null, null, null);
+ FieldConfig blob = new FieldConfig("blob", FieldConfig.EncodingType.RAW,
null, null, null);
+ FieldConfig rawPayload = new FieldConfig("raw_payload",
FieldConfig.EncodingType.RAW, null, null, null);
OpenStructIndexConfig config = new OpenStructIndexConfig(false, null,
1000, null, 0.5, List.of(blob, rawPayload));
assertFalse(config.shouldUseDictionaryForKey("blob"));
assertFalse(config.shouldUseDictionaryForKey("raw_payload"));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
