Re: [I] [multistage] Subqueries return different results depending on where clause [pinot]
Jackie-Jiang commented on issue #12949: URL: https://github.com/apache/pinot/issues/12949#issuecomment-2071169310 @gortiz Can you help take a look at this query? -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Multi stage stats [pinot]
Jackie-Jiang commented on code in PR #12704:
URL: https://github.com/apache/pinot/pull/12704#discussion_r1575464681
##
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##
@@ -51,6 +59,7 @@
"segmentStatistics", "traceInfo", "partialResult"
})
public class BrokerResponseNative implements BrokerResponse {
Review Comment:
I'm trying to follow the changes in this class. This class represents the
response for v1 queries. Currently (before this PR) we have a
`BrokerResponseNativeV2` which extends the v1 query response with just an
additional stage id stats list. I feel we should use this opportunity to clean
them up and decouple the v2 response format from the v1 format because a lot of
things do not apply for v2, and with v2 stats we don't really need the extra
fields in this class.
I guess the intention here is to keep things compatible with the current
interface. Shall we keep the changes within `BrokerResponseNativeV2` if
possible? We can make it directly implements `BrokerResponse` so that we can
easily decouple v1 and v2 in the future
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##
@@ -276,6 +264,161 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
return brokerResponse;
}
+ private void fillOldBrokerResponseStats(BrokerResponseNativeV2
brokerResponse,
+ List queryStats,
DispatchableSubPlan dispatchableSubPlan) {
+for (int i = 0; i < queryStats.size(); i++) {
+ MultiStageQueryStats.StageStats.Closed stageStats = queryStats.get(i);
+ if (stageStats == null) {
Review Comment:
For my understanding, will this ever contain `null` value? Is this for the
broker stage (stage 0)? If so, some comments can help understanding the code
##
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/ReceivingMailbox.java:
##
@@ -72,11 +81,53 @@ public String getId() {
return _id;
}
+ /**
+ * Offers a raw block into the mailbox within the timeout specified, returns
whether the block is successfully added.
+ * If the block is not added, an error block is added to the mailbox.
+ *
+ * Contrary to {@link #offer(TransferableBlock, long)}, the block may be an
+ * {@link TransferableBlock#isErrorBlock() error block}.
+ */
+ public ReceivingMailboxStatus offerRaw(ByteBuffer byteBuffer, long timeoutMs)
+ throws IOException {
+TransferableBlock block;
+long now = System.currentTimeMillis();
+_stats.merge(StatKey.WAIT_CPU_TIME_MS, now - _lastArriveTime);
+_lastArriveTime = now;
+_stats.merge(StatKey.DESERIALIZED_BYTES, byteBuffer.remaining());
+_stats.merge(StatKey.DESERIALIZED_MESSAGES, 1);
+
+now = System.currentTimeMillis();
+DataBlock dataBlock = DataBlockUtils.getDataBlock(byteBuffer);
+_stats.merge(StatKey.DESERIALIZATION_TIME_MS, System.currentTimeMillis() -
now);
+
+if (dataBlock instanceof MetadataBlock) {
+ Map exceptions = dataBlock.getExceptions();
+ if (exceptions.isEmpty()) {
+block = TransferableBlockUtils.wrap(dataBlock);
+ } else {
+
setErrorBlock(TransferableBlockUtils.getErrorTransferableBlock(exceptions));
+return ReceivingMailboxStatus.FIRST_ERROR;
Review Comment:
`FIRST_ERROR` is a little bit confusing. I think this means the error block
is successfully set? Maybe make it more explicit like `ERROR_BLOCK_SET`?
##
pinot-common/src/main/java/org/apache/pinot/common/datablock/V1MetadataBlock.java:
##
@@ -0,0 +1,165 @@
+/**
+ * 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.common.datablock;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.util.HashMap;
+import java.util.Map;
+
+
(pinot) branch master updated: handle absent segments so that catchup checker doesn't get stuck on them (#12883)
This is an automated email from the ASF dual-hosted git repository.
xbli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 8e10320595 handle absent segments so that catchup checker doesn't get
stuck on them (#12883)
8e10320595 is described below
commit 8e103205955e8af4fe286ebd6e97b30605724be2
Author: Xiaobing <61892277+klsi...@users.noreply.github.com>
AuthorDate: Mon Apr 22 16:41:46 2024 -0700
handle absent segments so that catchup checker doesn't get stuck on them
(#12883)
* skip missing segments while checking freshness during server startup
* get new consuming segments again if current consuming segments are
committed by other servers
---
.../server/starter/helix/BaseServerStarter.java| 71 +++-
.../FreshnessBasedConsumptionStatusChecker.java| 7 +-
.../IngestionBasedConsumptionStatusChecker.java| 128 ++---
.../helix/OffsetBasedConsumptionStatusChecker.java | 7 +-
.../helix/ConsumptionStatusCheckerTestUtils.java | 38 ++
...FreshnessBasedConsumptionStatusCheckerTest.java | 103 ++---
.../OffsetBasedConsumptionStatusCheckerTest.java | 32 --
7 files changed, 288 insertions(+), 98 deletions(-)
diff --git
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
index 02c7b81ea5..78cd1a14e7 100644
---
a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
+++
b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java
@@ -25,6 +25,7 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
@@ -153,8 +154,8 @@ public abstract class BaseServerStarter implements
ServiceStartable {
_helixClusterName =
_serverConf.getProperty(CommonConstants.Helix.CONFIG_OF_CLUSTER_NAME);
ServiceStartableUtils.applyClusterConfig(_serverConf, _zkAddress,
_helixClusterName, ServiceRole.SERVER);
-PinotInsecureMode.setPinotInInsecureMode(
-
Boolean.valueOf(_serverConf.getProperty(CommonConstants.CONFIG_OF_PINOT_INSECURE_MODE,
+PinotInsecureMode.setPinotInInsecureMode(Boolean.parseBoolean(
+_serverConf.getProperty(CommonConstants.CONFIG_OF_PINOT_INSECURE_MODE,
CommonConstants.DEFAULT_PINOT_INSECURE_MODE)));
setupHelixSystemProperties();
@@ -275,8 +276,7 @@ public abstract class BaseServerStarter implements
ServiceStartable {
// collect all resources which have this instance in the ideal state
List resourcesToMonitor = new ArrayList<>();
-
-Set consumingSegments = new HashSet<>();
+Map> consumingSegments = new HashMap<>();
boolean checkRealtime = realtimeConsumptionCatchupWaitMs > 0;
if (isFreshnessStatusCheckerEnabled && realtimeMinFreshnessMs <= 0) {
LOGGER.warn("Realtime min freshness {} must be > 0. Setting relatime min
freshness to default {}.",
@@ -289,23 +289,22 @@ public abstract class BaseServerStarter implements
ServiceStartable {
if (!TableNameBuilder.isTableResource(resourceName)) {
continue;
}
-
// Only monitor enabled resources
IdealState idealState =
_helixAdmin.getResourceIdealState(_helixClusterName, resourceName);
- if (idealState.isEnabled()) {
-
-for (String partitionName : idealState.getPartitionSet()) {
- if (idealState.getInstanceSet(partitionName).contains(_instanceId)) {
-resourcesToMonitor.add(resourceName);
-break;
- }
+ if (idealState == null || !idealState.isEnabled()) {
+continue;
+ }
+ for (String partitionName : idealState.getPartitionSet()) {
+if (idealState.getInstanceSet(partitionName).contains(_instanceId)) {
+ resourcesToMonitor.add(resourceName);
+ break;
}
-if (checkRealtime &&
TableNameBuilder.isRealtimeTableResource(resourceName)) {
- for (String partitionName : idealState.getPartitionSet()) {
-if (StateModel.SegmentStateModel.CONSUMING.equals(
-
idealState.getInstanceStateMap(partitionName).get(_instanceId))) {
- consumingSegments.add(partitionName);
-}
+ }
+ if (checkRealtime &&
TableNameBuilder.isRealtimeTableResource(resourceName)) {
+for (String partitionName : idealState.getPartitionSet()) {
+ if (StateModel.SegmentStateModel.CONSUMING.equals(
+ idealState.getInstanceStateMap(partitionName).get(_instanceId)))
{
+consumingSegments.computeIfAbsent(resourceName, k -> new
HashSet<>()).add(partitionName);
}
}
}
@@ -332,7
Re: [PR] handle absent segments so that catchup checker doesn't get stuck on them [pinot]
klsince merged PR #12883: URL: https://github.com/apache/pinot/pull/12883 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Upgrade Calcite to 1.36.0 [pinot]
robertzych commented on PR #12754: URL: https://github.com/apache/pinot/pull/12754#issuecomment-2071110780 I just filed the Calcite issue: https://issues.apache.org/jira/browse/CALCITE-6381 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Support NOT in StarTree Index [pinot]
codecov-commenter commented on PR #12988: URL: https://github.com/apache/pinot/pull/12988#issuecomment-2071048585 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12988?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `0%` with `187 lines` in your changes are missing coverage. Please review. > Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`d5bff4e`)](https://app.codecov.io/gh/apache/pinot/pull/12988?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 350 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/pinot/pull/12988?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [.../org/apache/pinot/core/startree/StarTreeUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fstartree%2FStarTreeUtils.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9TdGFyVHJlZVV0aWxzLmphdmE=) | 0.00% | [54 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...lter/predicate/RangePredicateEvaluatorFactory.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2Fpredicate%2FRangePredicateEvaluatorFactory.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL1JhbmdlUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | 0.00% | [32 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...core/startree/operator/StarTreeFilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fstartree%2Foperator%2FStarTreeFilterOperator.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zdGFydHJlZS9vcGVyYXRvci9TdGFyVHJlZUZpbHRlck9wZXJhdG9yLmphdmE=) | 0.00% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...core/operator/filter/predicate/PredicateUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2Fpredicate%2FPredicateUtils.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL1ByZWRpY2F0ZVV0aWxzLmphdmE=) | 0.00% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...edicate/BaseDictionaryBasedPredicateEvaluator.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2Fpredicate%2FBaseDictionaryBasedPredicateEvaluator.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL0Jhc2VEaWN0aW9uYXJ5QmFzZWRQcmVkaWNhdGVFdmFsdWF0b3IuamF2YQ==) | 0.00% | [17 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...lter/predicate/NotInPredicateEvaluatorFactory.java](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ffilter%2Fpredicate%2FNotInPredicateEvaluatorFactory.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvcHJlZGljYXRlL05vdEluUHJlZGljYXRlRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12988?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | |
Re: [PR] Upgrade lucene to 9.10.0 and compatibility changes to code. [pinot]
Jackie-Jiang commented on PR #12866: URL: https://github.com/apache/pinot/pull/12866#issuecomment-2071043920 I don't fully follow why did it break. Following the same logic in #11857 should work. Basically when downgrading and Pinot doesn't understand the new format, it should try to generate the old format -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add threadLocal reusable byte array for VarByteChunkReaderV4 [pinot]
itschrispeck commented on PR #12977: URL: https://github.com/apache/pinot/pull/12977#issuecomment-2071036509 Have you found any issues from these allocations? In our experience w/ V4 we have not seen GC issues even when using lower throughput GC algorithms (ZGC/generational ZGC). I remember there was a discussion/choice to avoid reusing the bytes previously: https://github.com/apache/pinot/pull/7661#discussion_r745500379 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back pinot spi shading [pinot]
Jackie-Jiang commented on PR #12969: URL: https://github.com/apache/pinot/pull/12969#issuecomment-2071014624 Solved with #12979 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back pinot spi shading [pinot]
Jackie-Jiang closed pull request #12969: Add back pinot spi shading URL: https://github.com/apache/pinot/pull/12969 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [I] Upsert table backfill enhancement: support externally partitioned data [pinot]
Jackie-Jiang commented on issue #12987: URL: https://github.com/apache/pinot/issues/12987#issuecomment-2071013541 For real-time ingested data, the partition must match the upstream partition id to ensure the upsert assumption of all data of the same partition served by the same server, and I don't think we can loose this requirement. `Partition function` is required for partition pruning. If partition pruning is not required, then we may allow custom partition id without a partition function. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Support NOT in StarTree Index [pinot]
Jackie-Jiang opened a new pull request, #12988: URL: https://github.com/apache/pinot/pull/12988 Support NOT in StarTree Index when it is followed by predicate or NOT (not with nested AND/OR) E.g. - ` WHERE NOT d1 > 10` - ` WHERE d1 > 10 AND NOT d2 < 10` - ` WHERE d1__COLUMN_NAME > 50 OR NOT d1__COLUMN_NAME > 10` - ` WHERE NOT NOT d1 > 10` - ` WHERE d2 < 95 AND (NOT d1 > 10 OR d1 > 50)` - ` WHERE d2 < 95 AND NOT d2 < 25 AND (d1 > 10 OR d1 < 50)` - ` WHERE NOT d1 = 95 AND (d1 > 90 OR d1 < 100)` In order to support NOT, dictionary based `BasePredicateEvaluator` are modified to support `getNonMatchingDictIds()`. Some refactor is done to avoid duplicate code. Bugfix: - Fix the unsorted dict ids returned from IN/NOT_IN predicate evaluator -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] [WIP] add locking logic to get consistent table view for upsert tables [pinot]
tibrewalpratik17 commented on code in PR #12976:
URL: https://github.com/apache/pinot/pull/12976#discussion_r1575309595
##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##
@@ -1103,6 +1210,48 @@ private static MutableRoaringBitmap
getQueryableDocIdsSnapshotFromSegment(IndexS
return queryableDocIdsSnapshot;
}
+ private void setSegmentContexts(List segmentContexts) {
+for (SegmentContext segmentContext : segmentContexts) {
+ IndexSegment segment = segmentContext.getIndexSegment();
+ if (_trackedSegments.contains(segment)) {
+
segmentContext.setQueryableDocIdsSnapshot(getQueryableDocIdsSnapshotFromSegment(segment));
+ }
+}
+ }
+
+ private boolean skipUpsertViewRefresh(long upsertViewFreshnessMs) {
+long nowMs = System.currentTimeMillis();
+if (upsertViewFreshnessMs >= 0) {
+ return _lastUpsertViewRefreshTimeMs + upsertViewFreshnessMs > nowMs;
+}
+return _lastUpsertViewRefreshTimeMs + _upsertViewRefreshIntervalMs > nowMs;
+ }
+
+ private void doBatchRefreshUpsertView(long upsertViewFreshnessMs) {
Review Comment:
Instead of doing this at partition metadata manager level can we move this
to table data manager level? We can maintain a map of segment -> segmentContext
and do a refresh there. This way we can easily extend the scope of segment
context in future as well.
I was trying on something like this --
https://github.com/apache/pinot/compare/master...tibrewalpratik17:pinot:fix_upsert_inconsistency?expand=1
##
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##
@@ -75,6 +75,15 @@ public enum Strategy {
@JsonPropertyDescription("Whether to preload segments for fast upsert
metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("Whether to enable consistent view for upsert
table")
+ private boolean _enableUpsertView;
+
+ @JsonPropertyDescription("Whether to enable batch refresh mode to keep
consistent view for upsert table")
+ private boolean _enableUpsertViewBatchRefresh;
Review Comment:
imo this should be enabled by default. At present, this is more of a bugfix
rather than a feature to be kept behind a config. It can be kept behind a
config initially to be on the safer side but then we should enable it by
default in the long run.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[I] Upsert table backfill enhancement: support externally partitioned data [pinot]
rohityadav1993 opened a new issue, #12987: URL: https://github.com/apache/pinot/issues/12987 ## Problem #6567 allows uploading a batch generated segment to Pinot upsert realtime table. Partitioned data is handled by defining the partition column in `segmentPartitionConfig.columnPartitionMap`. The addSegment flow uses the config to identify the partition value of the column in metadata.properties of the uplaoded segment and then assign the segment to instance based on the partition id and instance assignment zk metadata. This puts a restriction to define a partition column that is part of the table column(primary key) and also use only the configured [partition function](https://github.com/apache/pinot/blob/a5c728f549fe1be5560a88080caaa2063def3d87/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunctionFactory.java#L31) for partitioning data during segment creating in batch job using `SegmentIndexCreationDriverImpl` This restriction is not applicable for stream generated segments during realtime ingestion. The partition id is identified using the LLC segment name convention. The stream is partitioned externally and Pinot table does not need to be aware of the partitioned column/function. ## Proposal Proposal to enhance `SegmentAssignment.assignSegment()` flow to rely on externally provided partition id for an uplaoded segment. 1. Need to persist the partition id as part of segment zk metadata. 2. Modify the `StrictRealtimeSegmentAssignment.assignSegment` to get partition id from zk metadata. 3. Provide partition id externally: 1. Option 1: Provide partition id as http headers during segment upload 2. Option 2: Provide partition id as part of uploaded segment metadata(not as columnPartitionMap) (metadata.properties) Related: #10896, #11914 -- 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-unsubscr...@pinot.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch master updated (a852c8a424 -> a5c728f549)
This is an automated email from the ASF dual-hosted git repository. mcvsubbu pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pinot.git from a852c8a424 Update ORC and Hive dependency versions in the license binary file (#12986) add a5c728f549 Add back profile for shade (#12979) No new revisions were added by this update. Summary of changes: pinot-clients/pinot-jdbc-client/pom.xml | 15 ++- pinot-common/pom.xml | 11 ++- pinot-core/pom.xml | 11 +++ pinot-plugins/pinot-file-system/pinot-s3/pom.xml | 16 +++- .../pinot-stream-ingestion/pinot-kinesis/pom.xml | 16 +++- pinot-spi/pom.xml| 11 +++ 6 files changed, 76 insertions(+), 4 deletions(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back shade profile for shade removed in #12712 [pinot]
mcvsubbu merged PR #12979: URL: https://github.com/apache/pinot/pull/12979 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Issue #12367 testCaseTransformFunctionWithIntResults test fix [pinot]
Jackie-Jiang commented on code in PR #12922:
URL: https://github.com/apache/pinot/pull/12922#discussion_r1575173156
##
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java:
##
@@ -116,6 +145,90 @@ public void testCaseTransformFunctionWithIntResults() {
}
}
+ @Test
+ public void testCaseTransformFunctionWithoutCastForFloatValues() throws
Exception {
+Map dataSourceMap = new HashMap<>();
+int numRows = 1;
+final int[] intSVValues = new int[numRows];
Review Comment:
(minor, convention) we don't usually put `final` for local variable
##
pinot-core/src/test/java/org/apache/pinot/core/operator/transform/function/CaseTransformFunctionTest.java:
##
@@ -116,6 +145,90 @@ public void testCaseTransformFunctionWithIntResults() {
}
}
+ @Test
+ public void testCaseTransformFunctionWithoutCastForFloatValues() throws
Exception {
Review Comment:
(formatting) Can you reformat the changes with [Pinot
Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#set-up-ide)
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Issue #12367 testCaseTransformFunctionWithIntResults test fix [pinot]
aditya0811 commented on PR #12922: URL: https://github.com/apache/pinot/pull/12922#issuecomment-2070471323 Hello @Jackie-Jiang, please let me know your thoughts on the newly added negative scenario test. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Custom configuration property reader for segment metadata files [pinot]
abhioncbr commented on code in PR #12440:
URL: https://github.com/apache/pinot/pull/12440#discussion_r1575127192
##
pinot-spi/src/main/java/org/apache/pinot/spi/env/PropertyIOFactoryKind.java:
##
@@ -0,0 +1,25 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.env;
+
+public enum PropertyIOFactoryKind {
Review Comment:
I plan to use these enums to define table/cluster configs. I want to keep
these enums as of now, and if, in the next step of work, found not very
helpful, I will delete them. Thanks
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch master updated (c9d513aef0 -> a852c8a424)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pinot.git from c9d513aef0 Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 (#12983) add a852c8a424 Update ORC and Hive dependency versions in the license binary file (#12986) No new revisions were added by this update. Summary of changes: LICENSE-binary | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Update ORC and Hive dependency versions in the license binary file [pinot]
Jackie-Jiang merged PR #12986: URL: https://github.com/apache/pinot/pull/12986 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 [pinot]
Jackie-Jiang merged PR #12983: URL: https://github.com/apache/pinot/pull/12983 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch master updated: Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 (#12983)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/pinot.git The following commit(s) were added to refs/heads/master by this push: new c9d513aef0 Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 (#12983) c9d513aef0 is described below commit c9d513aef03774b5ea92020cf05b37f7ee7c72f2 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> AuthorDate: Mon Apr 22 10:19:50 2024 -0700 Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 (#12983) --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 79df63429c..a4a83c8b28 100644 --- a/pom.xml +++ b/pom.xml @@ -130,7 +130,7 @@ org.apache.pinot.shaded -3.4.0 +3.4.1 3.5.2 none - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch dependabot/maven/org.apache.maven.plugins-maven-jar-plugin-3.4.1 deleted (was a6c9ec1350)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch dependabot/maven/org.apache.maven.plugins-maven-jar-plugin-3.4.1 in repository https://gitbox.apache.org/repos/asf/pinot.git was a6c9ec1350 Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 The revisions that were on this branch are still contained in other references; therefore, this change does not discard any commits from the repository. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Update ORC and Hive dependency versions in the license binary file [pinot]
codecov-commenter commented on PR #12986: URL: https://github.com/apache/pinot/pull/12986#issuecomment-2070261379 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12986?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 62.06%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`e8753f6`)](https://app.codecov.io/gh/apache/pinot/pull/12986?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 347 commits behind head on master. Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #12986 +/- ## + Coverage 61.75% 62.06% +0.30% + Complexity 207 198 -9 Files 2436 2502 +66 Lines133233 136503+3270 Branches 2063621128 +492 + Hits 8227484716+2442 - Misses4491145511 +600 - Partials 6048 6276 +228 ``` | [Flag](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <ø> (-0.01%)` | :arrow_down: | | [integration](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <ø> (-0.01%)` | :arrow_down: | | [integration1](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <ø> (-0.01%)` | :arrow_down: | | [integration2](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `0.00% <ø> (ø)` | | | [java-11](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <ø> (-61.71%)` | :arrow_down: | | [java-21](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `62.06% <ø> (+0.43%)` | :arrow_up: | | [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `62.03% <ø> (+0.28%)` | :arrow_up: | | [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `62.02% <ø> (+34.30%)` | :arrow_up: | | [temurin](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `62.06% <ø> (+0.30%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `62.05% <ø> (+0.31%)` | :arrow_up: | | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `46.57% <ø> (-0.32%)` | :arrow_down: | | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12986/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `27.93% <ø> (+0.20%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12986?dropdown=coverage=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). -- This is an
Re: [PR] Add schema as input to the decoder. [pinot]
Jackie-Jiang commented on code in PR #12981:
URL: https://github.com/apache/pinot/pull/12981#discussion_r1575052394
##
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamMessageDecoder.java:
##
@@ -46,9 +47,25 @@ public interface StreamMessageDecoder {
* @param topicName Topic name of the stream
* @throws Exception If an error occurs
*/
+ @Deprecated
void init(Map props, Set fieldsToRead, String
topicName)
throws Exception;
+ /**
+ * Initializes the decoder. This is the new interface that should be
implemented by all decoders.
+ *
+ * @param props Decoder properties extracted from the {@link StreamConfig}
+ * @param fieldsToRead The fields to read from the source stream. If blank,
reads all fields (only for AVRO/JSON
+ * currently)
+ * @param topicName Topic name of the stream
+ * @param tableSchema Pinot schema of the table
+ * @throws Exception If an error occurs
+ */
+ default void init(Map props, Set fieldsToRead,
String topicName, Schema tableSchema)
Review Comment:
Let's just pass in `void init(StreamConfig streamConfig, TableConfig
tableConfig, Schema schema)` since everything is extracted from them.
##
pinot-spi/src/main/java/org/apache/pinot/spi/stream/StreamMessageDecoder.java:
##
@@ -46,9 +47,25 @@ public interface StreamMessageDecoder {
* @param topicName Topic name of the stream
* @throws Exception If an error occurs
*/
+ @Deprecated
Review Comment:
Since we are deprecating it, add a default implementation which throws
`UnsupportedOperationException`, then we can only choose to implement one of
the `init()` and clean up the decoders maintained within the code base
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
Jackie-Jiang commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1575044559
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##
@@ -102,7 +107,23 @@ public enum BrokerMeter implements AbstractMetrics.Meter {
NETTY_CONNECTION_BYTES_RECEIVED("nettyConnection", true),
PROACTIVE_CLUSTER_CHANGE_CHECK("proactiveClusterChangeCheck", true),
- DIRECT_MEMORY_OOM("directMemoryOOMCount", true);
+ DIRECT_MEMORY_OOM("directMemoryOOMCount", true),
+
+
+ /**
+ * Number of multi-stage queries that have been started.
+ *
+ * Unlike {@link #MULTI_STAGE_QUERIES_BY_TABLE}, this metric is global and
not attached to a particular table.
+ * That means it can be used to know how many multi-stage queries have been
started in total.
+ */
+ MULTI_STAGE_QUERIES_EXECUTED("queries", true),
Review Comment:
Also way may put them next to `QUERIES`
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
Jackie-Jiang commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1575043817
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##
@@ -102,7 +107,23 @@ public enum BrokerMeter implements AbstractMetrics.Meter {
NETTY_CONNECTION_BYTES_RECEIVED("nettyConnection", true),
PROACTIVE_CLUSTER_CHANGE_CHECK("proactiveClusterChangeCheck", true),
- DIRECT_MEMORY_OOM("directMemoryOOMCount", true);
+ DIRECT_MEMORY_OOM("directMemoryOOMCount", true),
+
+
+ /**
+ * Number of multi-stage queries that have been started.
+ *
+ * Unlike {@link #MULTI_STAGE_QUERIES_BY_TABLE}, this metric is global and
not attached to a particular table.
+ * That means it can be used to know how many multi-stage queries have been
started in total.
+ */
+ MULTI_STAGE_QUERIES_EXECUTED("queries", true),
Review Comment:
Suggest renaming them to `MULTI_STAGE_QUERIES_GLOBAL` and
`MULTI_STAGE_QUERIES`
##
pinot-common/src/main/java/org/apache/pinot/common/metrics/BrokerMeter.java:
##
@@ -85,7 +90,7 @@ public enum BrokerMeter implements AbstractMetrics.Meter {
REQUEST_DROPPED_DUE_TO_ACCESS_ERROR("requestsDropped", false),
GROUP_BY_SIZE("queries", false),
- TOTAL_SERVER_RESPONSE_SIZE("queries", false),
+ TOTAL_SERVER_RESPONSE_SIZE("bytes", false),
Review Comment:
Will this potentially cause backward compatible issue?
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Update ORC and Hive dependency versions in the license binary file [pinot]
yashmayya opened a new pull request, #12986: URL: https://github.com/apache/pinot/pull/12986 - The ORC and Hive dependency versions were upgraded in https://github.com/apache/pinot/pull/12956. This patch updates the license binary file with the new versions. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch dependabot/maven/aws.sdk.version-2.25.35 deleted (was 469990d428)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch dependabot/maven/aws.sdk.version-2.25.35 in repository https://gitbox.apache.org/repos/asf/pinot.git was 469990d428 Bump aws.sdk.version from 2.25.34 to 2.25.35 The revisions that were on this branch are still contained in other references; therefore, this change does not discard any commits from the repository. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch master updated (f83e466c42 -> 7b68aa369d)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pinot.git from f83e466c42 Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 (#12985) add 7b68aa369d Bump aws.sdk.version from 2.25.34 to 2.25.35 (#12984) No new revisions were added by this update. Summary of changes: pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Bump aws.sdk.version from 2.25.34 to 2.25.35 [pinot]
Jackie-Jiang merged PR #12984: URL: https://github.com/apache/pinot/pull/12984 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch master updated (e1b0e5357e -> f83e466c42)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch master in repository https://gitbox.apache.org/repos/asf/pinot.git from e1b0e5357e Refactor PinotTaskManager class (#12964) add f83e466c42 Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 (#12985) No new revisions were added by this update. Summary of changes: pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch dependabot/maven/org.roaringbitmap-RoaringBitmap-1.0.6 deleted (was ccfb0d3a55)
This is an automated email from the ASF dual-hosted git repository. jackie pushed a change to branch dependabot/maven/org.roaringbitmap-RoaringBitmap-1.0.6 in repository https://gitbox.apache.org/repos/asf/pinot.git was ccfb0d3a55 Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 The revisions that were on this branch are still contained in other references; therefore, this change does not discard any commits from the repository. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 [pinot]
Jackie-Jiang merged PR #12985: URL: https://github.com/apache/pinot/pull/12985 -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Metric for count of tables configured with various tier backends [pinot]
shounakmk219 commented on code in PR #12940:
URL: https://github.com/apache/pinot/pull/12940#discussion_r1574879993
##
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##
@@ -135,6 +137,13 @@ protected void postprocess(Context context) {
_controllerMetrics.setValueOfGlobalGauge(ControllerGauge.UPSERT_TABLE_COUNT,
context._upsertTableCount);
_controllerMetrics.setValueOfGlobalGauge(ControllerGauge.DISABLED_TABLE_COUNT,
context._disabledTables.size());
+// metric for total number of tables using a particular tier backend
+context._tierBackendTableCountMap.forEach((tier, count) ->
+ _controllerMetrics.setOrUpdateGauge(tier +
ControllerGauge.TIER_BACKEND_TABLE_COUNT.getGaugeName(), count));
Review Comment:
Good point! Tried to address it by maintaining the gauge names. Could not
think of a cleaner way to handle it.
##
pinot-controller/src/main/java/org/apache/pinot/controller/helix/SegmentStatusChecker.java:
##
@@ -135,6 +137,13 @@ protected void postprocess(Context context) {
_controllerMetrics.setValueOfGlobalGauge(ControllerGauge.UPSERT_TABLE_COUNT,
context._upsertTableCount);
_controllerMetrics.setValueOfGlobalGauge(ControllerGauge.DISABLED_TABLE_COUNT,
context._disabledTables.size());
+// metric for total number of tables using a particular tier backend
+context._tierBackendTableCountMap.forEach((tier, count) ->
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch dependabot/maven/org.roaringbitmap-RoaringBitmap-1.0.6 created (now ccfb0d3a55)
This is an automated email from the ASF dual-hosted git repository. github-bot pushed a change to branch dependabot/maven/org.roaringbitmap-RoaringBitmap-1.0.6 in repository https://gitbox.apache.org/repos/asf/pinot.git at ccfb0d3a55 Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Bump org.roaringbitmap:RoaringBitmap from 1.0.5 to 1.0.6 [pinot]
dependabot[bot] opened a new pull request, #12985: URL: https://github.com/apache/pinot/pull/12985 Bumps [org.roaringbitmap:RoaringBitmap](https://github.com/RoaringBitmap/RoaringBitmap) from 1.0.5 to 1.0.6. Release notes Sourced from https://github.com/RoaringBitmap/RoaringBitmap/releases;>org.roaringbitmap:RoaringBitmap's releases. 1.0.6 What's Changed Implement BatchIterator's promise to fill the input buffer, when possible by https://github.com/smmathews-bw-boston;>@smmathews-bw-boston in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/712;>RoaringBitmap/RoaringBitmap#712 RoaringBitmap to BitSet/long[]/byte[] by https://github.com/DanielThomas;>@DanielThomas in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/713;>RoaringBitmap/RoaringBitmap#713 Avoid getCardinality in RunContainer.toBitmapContainer by https://github.com/DanielThomas;>@DanielThomas in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/715;>RoaringBitmap/RoaringBitmap#715 ADD Java 21 to CI by https://github.com/smmathews-bw-boston;>@smmathews-bw-boston in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/719;>RoaringBitmap/RoaringBitmap#719 Avoid intermediate arrays for other container types by https://github.com/DanielThomas;>@DanielThomas in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/716;>RoaringBitmap/RoaringBitmap#716 Fix for or:ing two RunContainers that produce an undersized BitmapContainer by https://github.com/larsk-db;>@larsk-db in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/718;>RoaringBitmap/RoaringBitmap#718 New Contributors https://github.com/smmathews-bw-boston;>@smmathews-bw-boston made their first contribution in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/712;>RoaringBitmap/RoaringBitmap#712 https://github.com/DanielThomas;>@DanielThomas made their first contribution in https://redirect.github.com/RoaringBitmap/RoaringBitmap/pull/713;>RoaringBitmap/RoaringBitmap#713 Full Changelog: https://github.com/RoaringBitmap/RoaringBitmap/compare/1.0.5...1.0.6;>https://github.com/RoaringBitmap/RoaringBitmap/compare/1.0.5...1.0.6 Commits https://github.com/RoaringBitmap/RoaringBitmap/commit/5235aa62c32fa3bf7fae40a562e3edc75f61be4e;>5235aa6 [Gradle Release Plugin] - pre tag commit: '1.0.6'. https://github.com/RoaringBitmap/RoaringBitmap/commit/0735196d60eec42324f1c356da56f001bad67c39;>0735196 Fix for or:ing two RunContainers that produce an undersized BitmapContainer (... https://github.com/RoaringBitmap/RoaringBitmap/commit/bea55d46dfe97c3e76d67f26a335c31014f04d77;>bea55d4 Avoid intermediate arrays for other container types (https://redirect.github.com/RoaringBitmap/RoaringBitmap/issues/716;>#716) https://github.com/RoaringBitmap/RoaringBitmap/commit/0905e734a4fb3d5abf0b1d9fb3859a08c4af7c41;>0905e73 add Java 21 to CI, as one of four LTS versions still being supported by Eclip... https://github.com/RoaringBitmap/RoaringBitmap/commit/91ff377e89672170a5cda4b2ed0ec5c4ce1f21bd;>91ff377 Update README.md https://github.com/RoaringBitmap/RoaringBitmap/commit/9079e41f913842d4ce53eec4f36c0c14e41dd587;>9079e41 Avoid getCardinality in RunContainer.toBitmapContainer (https://redirect.github.com/RoaringBitmap/RoaringBitmap/issues/715;>#715) https://github.com/RoaringBitmap/RoaringBitmap/commit/01fad43ae9a3f52bf285f6fba36febef4967392e;>01fad43 RoaringBitmap to BitSet/long[]/byte[] (https://redirect.github.com/RoaringBitmap/RoaringBitmap/issues/713;>#713) https://github.com/RoaringBitmap/RoaringBitmap/commit/dda04be1c1ff514b94875499476825daba4ed3dc;>dda04be Implement BatchIterator's promise to fill the input buffer, when possible (https://redirect.github.com/RoaringBitmap/RoaringBitmap/issues/712;>#712) https://github.com/RoaringBitmap/RoaringBitmap/commit/78da63af7eedb1ae118340d06974cfdc63cb94f6;>78da63a [Gradle Release Plugin] - new version commit: '1.0.6-SNAPSHOT'. See full diff in https://github.com/RoaringBitmap/RoaringBitmap/compare/1.0.5...1.0.6;>compare view [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to
(pinot) branch dependabot/maven/aws.sdk.version-2.25.35 created (now 469990d428)
This is an automated email from the ASF dual-hosted git repository. github-bot pushed a change to branch dependabot/maven/aws.sdk.version-2.25.35 in repository https://gitbox.apache.org/repos/asf/pinot.git at 469990d428 Bump aws.sdk.version from 2.25.34 to 2.25.35 No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Bump aws.sdk.version from 2.25.34 to 2.25.35 [pinot]
dependabot[bot] opened a new pull request, #12984: URL: https://github.com/apache/pinot/pull/12984 Bumps `aws.sdk.version` from 2.25.34 to 2.25.35. Updates `software.amazon.awssdk:bom` from 2.25.34 to 2.25.35 Updates `software.amazon.awssdk:apache-client` from 2.25.34 to 2.25.35 Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 [pinot]
dependabot[bot] opened a new pull request, #12983: URL: https://github.com/apache/pinot/pull/12983 Bumps [org.apache.maven.plugins:maven-jar-plugin](https://github.com/apache/maven-jar-plugin) from 3.4.0 to 3.4.1. Release notes Sourced from https://github.com/apache/maven-jar-plugin/releases;>org.apache.maven.plugins:maven-jar-plugin's releases. 3.4.1 Bug Fixes https://issues.apache.org/jira/browse/MJAR-307;>[MJAR-307] - Wrong version of commons-io cause a ClassNotFound o.a.commons.io.file.attribute.FileTimes (https://redirect.github.com/apache/maven-jar-plugin/pull/83;>#83) https://github.com/slawekjaranowski;>@slawekjaranowski Dependency updates https://issues.apache.org/jira/browse/MJAR-308;>[MJAR-308] - Bump org.apache.maven.plugins:maven-plugins from 41 to 42 (https://redirect.github.com/apache/maven-jar-plugin/pull/85;>#85) https://github.com/dependabot;>@dependabot Commits https://github.com/apache/maven-jar-plugin/commit/8b29adc0e420b1da1495791c9d5bb9399590cc51;>8b29adc [maven-release-plugin] prepare release maven-jar-plugin-3.4.1 https://github.com/apache/maven-jar-plugin/commit/325b2990308f08d4a2b3010cafe8bb55304ef53c;>325b299 [MJAR-308] Bump org.apache.maven.plugins:maven-plugins from 41 to 42 (https://redirect.github.com/apache/maven-jar-plugin/issues/85;>#85) https://github.com/apache/maven-jar-plugin/commit/52111cc9250fc5876f4a6570a48bc679d099d8e2;>52111cc [MJAR-307] Wrong version of commons-io cause a ClassNotFound o.a.commons.io.f... https://github.com/apache/maven-jar-plugin/commit/902d4c55a19f301c31a4e187fbd798d4f847cb19;>902d4c5 [maven-release-plugin] prepare for next development iteration See full diff in https://github.com/apache/maven-jar-plugin/compare/maven-jar-plugin-3.4.0...maven-jar-plugin-3.4.1;>compare view [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
(pinot) branch dependabot/maven/org.apache.maven.plugins-maven-jar-plugin-3.4.1 created (now a6c9ec1350)
This is an automated email from the ASF dual-hosted git repository. github-bot pushed a change to branch dependabot/maven/org.apache.maven.plugins-maven-jar-plugin-3.4.1 in repository https://gitbox.apache.org/repos/asf/pinot.git at a6c9ec1350 Bump org.apache.maven.plugins:maven-jar-plugin from 3.4.0 to 3.4.1 No new revisions were added by this update. - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
gortiz commented on PR #12982: URL: https://github.com/apache/pinot/pull/12982#issuecomment-2069098588 I've simplified the PR and modified the title and description of the PR. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
gortiz commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1574559296
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##
@@ -260,10 +276,18 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
brokerResponse.setPartialResult(isPartialResult(brokerResponse));
// Set total query processing time
-// TODO: Currently we don't emit metric for QUERY_TOTAL_TIME_MS
long totalTimeMs = TimeUnit.NANOSECONDS.toMillis(
sqlNodeAndOptions.getParseTimeNs() + (executionEndTimeNs -
compilationStartTimeNs));
brokerResponse.setTimeUsedMs(totalTimeMs);
+if (brokerResponse.isNumGroupsLimitReached()) {
+ for (String tableName : tableNames) {
+_brokerMetrics.addMeteredTableValue(tableName,
BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
+ }
Review Comment:
I think it is better to have _false positives_ (aka report the problem at
least once) than _false negatives_ (aka report it at most once), but given
there is no consensus, I'm going to remove it.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
gortiz commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1574558083
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##
@@ -309,7 +309,7 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
JsonNode request, @Nullable RequesterIdentity requesterIdentity,
RequestContext requestContext,
@Nullable HttpHeaders httpHeaders)
throws Exception {
-LOGGER.debug("SQL query for request {}: {}", requestId, query);
+LOGGER.info("SQL query for request {}: {}", requestId, query);
Review Comment:
I'm removing it given it is clear this is not a consensuated decision.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
codecov-commenter commented on PR #12982: URL: https://github.com/apache/pinot/pull/12982#issuecomment-2069078164 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12982?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: Patch coverage is `0%` with `25 lines` in your changes are missing coverage. Please review. > Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) to head [(`29288c7`)](https://app.codecov.io/gh/apache/pinot/pull/12982?dropdown=coverage=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). > Report is 345 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/pinot/pull/12982?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [...requesthandler/MultiStageBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frequesthandler%2FMultiStageBrokerRequestHandler.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvTXVsdGlTdGFnZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | 0.00% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...a/org/apache/pinot/common/metrics/BrokerMeter.java](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FBrokerMeter.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJNZXRlci5qYXZh) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree=pinot-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fbroker%2Frequesthandler%2FBaseBrokerRequestHandler.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12982?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #12982 +/- ## = - Coverage 61.75%0.00% -61.75% + Complexity 2076 -201 = Files 2436 2427-9 Lines133233 132880 -353 Branches 2063620584 -52 = - Hits 822746-82268 - Misses44911 132874+87963 + Partials 60480 -6048 ``` | [Flag](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | | [integration](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: | | [integration1](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `<0.01% <0.00%> (-0.01%)` | :arrow_down: | | [integration2](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `0.00% <0.00%> (ø)` | | | [java-11](https://app.codecov.io/gh/apache/pinot/pull/12982/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | |
Re: [PR] Add some multi-stage metrics [pinot]
gortiz commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1574536612
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##
@@ -309,7 +309,7 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
JsonNode request, @Nullable RequesterIdentity requesterIdentity,
RequestContext requestContext,
@Nullable HttpHeaders httpHeaders)
throws Exception {
-LOGGER.debug("SQL query for request {}: {}", requestId, query);
+LOGGER.info("SQL query for request {}: {}", requestId, query);
Review Comment:
This would just double the lines, given we already emit a info log for each
query in `QueryLogger.log`, which is executed at the end of the request.
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add some multi-stage metrics [pinot]
tibrewalpratik17 commented on code in PR #12982:
URL: https://github.com/apache/pinot/pull/12982#discussion_r1574511131
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##
@@ -309,7 +309,7 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
JsonNode request, @Nullable RequesterIdentity requesterIdentity,
RequestContext requestContext,
@Nullable HttpHeaders httpHeaders)
throws Exception {
-LOGGER.debug("SQL query for request {}: {}", requestId, query);
+LOGGER.info("SQL query for request {}: {}", requestId, query);
Review Comment:
This will add a lot of log lines for high-qps use-cases.
##
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##
@@ -260,10 +276,18 @@ protected BrokerResponse handleRequest(long requestId,
String query, @Nullable S
brokerResponse.setPartialResult(isPartialResult(brokerResponse));
// Set total query processing time
-// TODO: Currently we don't emit metric for QUERY_TOTAL_TIME_MS
long totalTimeMs = TimeUnit.NANOSECONDS.toMillis(
sqlNodeAndOptions.getParseTimeNs() + (executionEndTimeNs -
compilationStartTimeNs));
brokerResponse.setTimeUsedMs(totalTimeMs);
+if (brokerResponse.isNumGroupsLimitReached()) {
+ for (String tableName : tableNames) {
+_brokerMetrics.addMeteredTableValue(tableName,
BrokerMeter.BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED, 1);
+ }
Review Comment:
We were planning to do something similar before and had some reservations
around this. More info on this thread --
https://github.com/apache/pinot/pull/11023#discussion_r1248823295
cc @ankitsultana
--
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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
[PR] Add some multi-stage metrics [pinot]
gortiz opened a new pull request, #12982: URL: https://github.com/apache/pinot/pull/12982 Including: - Report BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED - Report QUERY_TOTAL_TIME_MS - Report QUERY_QUOTA_EXCEEDED - Report REQUEST_DROPPED_DUE_TO_ACCESS_ERROR - Report QUERIES - Report REQUEST_SIZE - Add and report new MULTI_STAGE_QUERIES_BY_TABLE Some javadocs have been added to notice that some of these metrics can now be counted now more than once (ie BROKER_RESPONSES_WITH_NUM_GROUPS_LIMIT_REACHED is added for each table touched in the query in case of joins) This PR also changes a log. Specifically, the sentence that logs that a query has started is changed from debug to info. It is very useful to have a log when the query starts and one when it ends (the later already existed). This is specially useful to detect queries that for whatever reason do not end (like producing a large GC that ends up killing the broker). -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back shade profile for shade removed in #12712 [pinot]
xiangfu0 commented on PR #12979: URL: https://github.com/apache/pinot/pull/12979#issuecomment-2068839101 It's disabled by default. Plugins modules are enabled by default. We added this profile to enable shading for modules like pinot-spi, or disable plugin modules like pinot-s3. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [I] Grouping data by time interval [pinot]
synoxbm commented on issue #48: URL: https://github.com/apache/pinot/issues/48#issuecomment-2068670535 [Jackie-Jiang](https://github.com/Jackie-Jiang) thanks ;) -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back shade profile for shade removed in #12712 [pinot]
gortiz commented on PR #12979: URL: https://github.com/apache/pinot/pull/12979#issuecomment-2068638072 So... IICU: - Shading is enabled by default - Some projects disable it - In some projects we can tune whether it is enabled or not by using a property Is that correct? PS: I guess we can discuss this in another PR but... why do we need shade to be on by default? I think we should only shade drivers and some plugins and at least it should be pretty safe to only turn shade on in profile `bin-dist`. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add back shade profile for shade removed in #12712 [pinot]
gortiz commented on code in PR #12979: URL: https://github.com/apache/pinot/pull/12979#discussion_r1574222460 ## pinot-clients/pinot-jdbc-client/pom.xml: ## @@ -82,4 +81,18 @@ jsr305 + + + build-shaded-jar + + + skipShade + !true + + + +package + Review Comment: Isn't that the default phase? IICU we don't need to provide the phase. ## pinot-clients/pinot-jdbc-client/pom.xml: ## @@ -82,4 +81,18 @@ jsr305 + + + build-shaded-jar + + + skipShade + !true + + + +package + Review Comment: nit: Isn't that the default phase? IICU we don't need to provide the phase. -- 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-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
