You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2025/03/19 18:02:30 UTC
[PR] Make stats system more resilient in MSE [pinot]
gortiz opened a new pull request, #15312:
URL: https://github.com/apache/pinot/pull/15312
Working on https://github.com/apache/pinot/pull/15245 I've detected an issue with how stats are generated in Pinot 1.3.0. The issue itself is not relevant here. Instead, the important part is that by fixing the issue, we break backward compatibility because Pinot 1.3.0 and lower fail if received stats from other stages different stat types are found. For example, if a worker sends a list of stats that start with FILTER and another sends stats that start with PROJECT.
This PR includes the following changes:
1. pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/BlockingMultiStreamConsumer.java now doesn't fail if different stats are received. Instead, it just ignores children's stats if that situation is detected.
2. Adds a new simple instance config in helix storing the Pinot version being used.
3. Adds a new config to specify whether stats should be sent to other stages or not.
4. Adds three strategies for that config:
- Never send stats
- Always send stats (the previous default)
- Safely send stats (the new default)
The safe mode listens to instance config changes in Helix. If all instances run a version considered compatible (in this case, it starts with 1.4 or higher), then stats are sent. Otherwise, they are not.
This means that upgrading from 1.3.0 to 1.4.0 should be backward compatible, although during the upgrade MSE queries will not include stats.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005376991
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/plan/pipeline/PipelineBreakerExecutorTest.java:
##########
@@ -101,6 +104,15 @@ public void tearDownMethod()
_mocks.close();
}
+ @VisibleForTesting
Review Comment:
I forgot to remove it :laughing:
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #15312:
URL: https://github.com/apache/pinot/pull/15312
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15312:
URL: https://github.com/apache/pinot/pull/15312#issuecomment-2737681961
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/15312?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `52.63158%` with `18 lines` in your changes missing coverage. Please review.
> Project coverage is 55.92%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`bd7f540`)](https://app.codecov.io/gh/apache/pinot/commit/bd7f540d5b348becdda6284f33ced08535392fea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1872 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/15312?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/apache/pinot/common/utils/helix/HelixHelper.java](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2Fhelix%2FHelixHelper.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvaGVsaXgvSGVsaXhIZWxwZXIuamF2YQ==) | 0.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...me/operator/utils/BlockingMultiStreamConsumer.java](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2Futils%2FBlockingMultiStreamConsumer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci91dGlscy9CbG9ja2luZ011bHRpU3RyZWFtQ29uc3VtZXIuamF2YQ==) | 53.33% | [5 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../org/apache/pinot/common/version/PinotVersion.java](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fversion%2FPinotVersion.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdmVyc2lvbi9QaW5vdFZlcnNpb24uamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/query/runtime/operator/MailboxSendOperator.java](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMailboxSendOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | 66.66% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/15312?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (bd7f540). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (bd7f540) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|0|
>|skip-bytebuffers-false|7|1|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #15312 +/- ##
============================================
- Coverage 61.75% 55.92% -5.83%
- Complexity 207 685 +478
============================================
Files 2436 2173 -263
Lines 133233 117269 -15964
Branches 20636 18753 -1883
============================================
- Hits 82274 65581 -16693
- Misses 44911 46280 +1369
+ Partials 6048 5408 -640
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <52.63%> (-5.71%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <52.63%> (-5.83%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <52.63%> (-5.83%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <52.63%> (-5.83%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.92% <52.63%> (+9.03%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/15312/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/15312?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005026509
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
+ return true;
+ }
+ // Lets try to parse 1.x versions
Review Comment:
I'm also adding a check to compare the value with the current version of the process. Given most of the cluster will be homogeneous, most of the time, it won't be necessary to parse the version
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005012571
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/BlockingMultiStreamConsumer.java:
##########
@@ -244,11 +244,14 @@ private E readBlockOrNull() {
public static class OfTransferableBlock extends BlockingMultiStreamConsumer<TransferableBlock> {
- private final MultiStageQueryStats _stats;
+ private final int _stageId;
+ @Nullable
+ private volatile MultiStageQueryStats _stats;
Review Comment:
You are right. I'm also adding Javadoc to specify which methods are allowed to be called by which threads
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005022698
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
Review Comment:
Done
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005375692
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/QueryRunner.java:
##########
@@ -130,13 +131,14 @@ public class QueryRunner {
private JoinOverFlowMode _joinOverflowMode;
@Nullable
private PhysicalTimeSeriesServerPlanVisitor _timeSeriesPhysicalPlanVisitor;
+ private BooleanSupplier _sendStats;
/**
* Initializes the query executor.
* <p>Should be called only once and before calling any other method.
*/
public void init(PinotConfiguration config, InstanceDataManager instanceDataManager, HelixManager helixManager,
- ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig) {
+ ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig, BooleanSupplier sendStats) {
Review Comment:
An engineer (in fact, a mathematician) I respect told me once that his primary requirement for a module to be considered _good enough_ is to be able to run several instances of it in the same process. Using singletons with state goes against that pattern. We need to do it sometimes because we cannot refactor Pinot right now.
Could we create an integration test with two servers with a different send stats configuration? In the model you propose, we couldn't.
Also, we cannot use SendStatsPredicate directly in pinot-query-runtime because it is declared in pinot-server, which depends on pinot-query-runtime. We could probably move SendStatsPredicate somewhere else, but finding a place where it _can_ be (in technical terms) and _should_ be (in semantic terms) doesn't seem trivial.
> Can you think of any strategy to avoid adding this extra parameter in so many places?
In Java, we have three options: Using thread locals, dependency injection, or context objects containing all required information. None makes much sense here, given we would want to change the parameters. If I had to choose one, DI would probably be the cleanest, but it would also require more changes in the code. In the past, we discussed that, but we thought it was a low priority high effort task
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2011055715
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1475,6 +1476,23 @@ public static class MultiStageQueryRunner {
public static final String KEY_OF_MAX_ROWS_IN_JOIN = "pinot.query.join.max.rows";
public static final String KEY_OF_JOIN_OVERFLOW_MODE = "pinot.query.join.overflow.mode";
+ /// Specifies the send stats mode used in MSE.
+ ///
+ /// Valid values are (in lower or upper case):
+ /// - "SAFE": MSE will only send stats if all instances in the cluster are running 1.4.0 or later.
+ /// - "ALWAYS": MSE will always send stats, regardless of the version of the instances in the cluster.
+ /// - "NEVER": MSE will never send stats.
+ ///
+ /// The reason for this flag that versions 1.3.0 and lower have two undesired behaviors:
+ /// 1. Some queries using intersection generate incorrect stats
+ /// 2. When stats from other nodes are sent but are different from expected, the query fails.
+ ///
+ /// In 1.4.0 the first issue is solved and instead of failing when unexpected stats are received, the query
+ /// continues without children stats. But if a query involves servers in versions 1.3.0 and 1.4.0, the one
+ /// running 1.3.0 may fail, which breaks backward compatibility.
+ public static final String KEY_OF_SEND_STATS_MODE = "pinot.query.mse.stats.mode";
+ public static final String DEFAULT_SEND_STATS_MODE = "safe";
Review Comment:
(minor) To be consistent with the enum
```suggestion
public static final String DEFAULT_SEND_STATS_MODE = "SAFE";
```
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005022953
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
Review Comment:
Nice! I'm changing it
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
Review Comment:
Removed
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2004365100
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/BlockingMultiStreamConsumer.java:
##########
@@ -244,11 +244,14 @@ private E readBlockOrNull() {
public static class OfTransferableBlock extends BlockingMultiStreamConsumer<TransferableBlock> {
- private final MultiStageQueryStats _stats;
+ private final int _stageId;
+ @Nullable
+ private volatile MultiStageQueryStats _stats;
Review Comment:
(minor) Is this class single-threaded? Do we need to keep thread safety?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1475,6 +1476,23 @@ public static class MultiStageQueryRunner {
public static final String KEY_OF_MAX_ROWS_IN_JOIN = "pinot.query.join.max.rows";
public static final String KEY_OF_JOIN_OVERFLOW_MODE = "pinot.query.join.overflow.mode";
+ /// Specifies the send stats mode used in MSE.
Review Comment:
(minor) Consider keeping the same javadoc convention for consistency
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
Review Comment:
This can be a little bit dangerous because stats will be disabled even if all versions are the same. Do you know when version is unavailable?
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
Review Comment:
(minor) Consider keeping the same javadoc convention for consistency
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -1475,6 +1476,23 @@ public static class MultiStageQueryRunner {
public static final String KEY_OF_MAX_ROWS_IN_JOIN = "pinot.query.join.max.rows";
public static final String KEY_OF_JOIN_OVERFLOW_MODE = "pinot.query.join.overflow.mode";
+ /// Specifies the send stats mode used in MSE.
+ ///
+ /// Valid values are:
+ /// - "safe": MSE will only send stats if all instances in the cluster are running 1.4.0 or later.
Review Comment:
(minor) Since it is enum, use upper case is more readable IMO
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
Review Comment:
Seems like we will never hit this line
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
Review Comment:
This could flood the log during rolling restart (server setting `shutDownInProgress` flag) in large cluster. Consider logging a summary line per callback
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
Review Comment:
This doesn't seems correct. We shouldn't skip `null` because `null` is unsafe
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
Review Comment:
You should be able to use `InstanceTypeUtils`
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
+ return true;
+ }
+ // Lets try to parse 1.x versions
Review Comment:
Do we need the following check? If the version is present, it is safe version right?
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
Review Comment:
(minor) Can we use `Mode.valueOf()`?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005011839
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
Review Comment:
Yes, it seems only reached when the code was packaged without Maven.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005029396
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
Review Comment:
I'm starting to use the new Javadoc markdown format because I think it is easier to read without IDE help (ie in GH reviews) while IDEs already recognize it (at least IntelliJ) even when Java 11 is being used.
For example, adding links and especially lists is quite easier to write and read this way.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "albertobastos (via GitHub)" <gi...@apache.org>.
albertobastos commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005236269
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/BlockingMultiStreamConsumer.java:
##########
@@ -244,11 +244,14 @@ private E readBlockOrNull() {
public static class OfTransferableBlock extends BlockingMultiStreamConsumer<TransferableBlock> {
- private final MultiStageQueryStats _stats;
+ private final int _stageId;
+ @Nullable
+ private volatile MultiStageQueryStats _stats;
public OfTransferableBlock(OpChainExecutionContext context,
List<? extends AsyncStream<TransferableBlock>> asyncProducers) {
super(context.getId(), context.getDeadlineMs(), asyncProducers);
+ _stageId = context.getStageId();
_stats = MultiStageQueryStats.emptyStats(context.getStageId());
Review Comment:
nit: use the already set `_stageId` variable
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/QueryRunner.java:
##########
@@ -130,13 +131,14 @@ public class QueryRunner {
private JoinOverFlowMode _joinOverflowMode;
@Nullable
private PhysicalTimeSeriesServerPlanVisitor _timeSeriesPhysicalPlanVisitor;
+ private BooleanSupplier _sendStats;
/**
* Initializes the query executor.
* <p>Should be called only once and before calling any other method.
*/
public void init(PinotConfiguration config, InstanceDataManager instanceDataManager, HelixManager helixManager,
- ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig) {
+ ServerMetrics serverMetrics, @Nullable TlsConfig tlsConfig, BooleanSupplier sendStats) {
Review Comment:
I was going to propose to have a global singleton for the `sendStats` boolean properly initialized by the `BaseServerStarter` and updated by the Helix Manager, but then I saw you want to use a custom supplier in just a few places...
Can you think of any strategy to avoid adding this extra parameter in so many places?
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
Review Comment:
Some javadoc here...
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/plan/pipeline/PipelineBreakerExecutorTest.java:
##########
@@ -101,6 +104,15 @@ public void tearDownMethod()
_mocks.close();
}
+ @VisibleForTesting
Review Comment:
Do we need the `@VisibleForTesting` annotation in a Test class? I'm guessing you forgot to remove it after copy-paste it from `PipelineBreakerExecutor`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Make stats system more resilient in MSE [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #15312:
URL: https://github.com/apache/pinot/pull/15312#discussion_r2005024421
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SendStatsPredicate.java:
##########
@@ -0,0 +1,160 @@
+/**
+ * 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.server.starter.helix;
+
+import java.util.List;
+import java.util.Locale;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.api.listeners.InstanceConfigChangeListener;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.pinot.common.version.PinotVersion;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.utils.CommonConstants;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public abstract class SendStatsPredicate implements InstanceConfigChangeListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(SendStatsPredicate.class);
+
+ public abstract boolean getSendStats();
+
+ public static SendStatsPredicate create(PinotConfiguration configuration) {
+ String modeStr = configuration.getProperty(
+ CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE,
+ CommonConstants.MultiStageQueryRunner.DEFAULT_SEND_STATS_MODE).toUpperCase(Locale.ENGLISH);
+ for (Mode mode : Mode.values()) {
+ if (mode.name().equals(modeStr)) {
+ return mode.create();
+ }
+ }
+
+ throw new IllegalStateException("Invalid value " + modeStr + " for "
+ + CommonConstants.MultiStageQueryRunner.KEY_OF_SEND_STATS_MODE);
+ }
+
+ public enum Mode {
+ SAFE {
+ @Override
+ public SendStatsPredicate create() {
+ return new Safe();
+ }
+ },
+ ALWAYS {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ },
+ NEVER {
+ @Override
+ public SendStatsPredicate create() {
+ return new SendStatsPredicate() {
+ @Override
+ public boolean getSendStats() {
+ return false;
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ // Nothing to do
+ }
+ };
+ }
+ };
+
+ public abstract SendStatsPredicate create();
+ }
+
+ private static class Safe extends SendStatsPredicate {
+ private final AtomicBoolean _sendStats = new AtomicBoolean(true);
+
+ @Override
+ public boolean getSendStats() {
+ return _sendStats.get();
+ }
+
+ @Override
+ public void onInstanceConfigChange(List<InstanceConfig> instanceConfigs, NotificationContext context) {
+ boolean sendStats = true;
+ // TODO: Right now we read all configs, while only broker and servers are actually important
+ // But it isn't clear how to detect for sure that an instance config belongs to a broker or server
+ for (InstanceConfig instanceConfig : instanceConfigs) {
+ String otherVersion = instanceConfig.getRecord()
+ .getStringField(CommonConstants.Helix.Instance.PINOT_VERSION_KEY, null);
+ if (otherVersion == null || otherVersion.equals(PinotVersion.UNKNOWN)) {
+ LOGGER.warn("Instance {} does not have version", instanceConfig.getInstanceName());
+ continue;
+ }
+ if (isProblematicVersion(otherVersion)) {
+ LOGGER.info("Found problematic version {} on instance {}. Stats may not be recognized", otherVersion,
+ instanceConfig.getInstanceName());
+ sendStats = false;
+ break;
+ }
+ }
+ if (_sendStats.getAndSet(sendStats) != sendStats) {
+ LOGGER.warn("Send MSE stats is now {}", sendStats ? "enabled" : "disabled");
+ }
+ }
+
+ /// Returns true if the version is problematic
+ ///
+ /// Ideally [PinotVersion] should have a way to extract versions in comparable format, but given it doesn't we
+ /// need to parse the string here. In case version doesn't match `1\.x\..*`, we treat is as a problematic version
+ private boolean isProblematicVersion(@Nullable String versionStr) {
+ if (versionStr == null) {
+ return true;
+ }
+ if (versionStr.equals(PinotVersion.UNKNOWN)) {
+ return true;
+ }
+ // Lets try to parse 1.x versions
Review Comment:
I'm just being extra careful, given this code is sensitive. We don't want to throw an array out of bound exception here just because for whatever reason we change the version format to something that, for example, doesn't include dots
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org