You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/03/21 23:01:01 UTC
[GitHub] [pinot] walterddr opened a new pull request, #10455: create segment zk metadata cache
walterddr opened a new pull request, #10455:
URL: https://github.com/apache/pinot/pull/10455
create segmentZKMetadataCache that only pulls once from ZK and then reused by all pruners.
TODO
- [x] only pull ZK once and reused by all pruners
- [ ] if no registered pruner, do not pull from zk
- [ ] maintain a seen list and unseen list of segments so it doesn't pull when not-necessary
- [ ] add tests
--
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
[GitHub] [pinot] walterddr merged pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10455:
URL: https://github.com/apache/pinot/pull/10455
--
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
[GitHub] [pinot] ankitsultana commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "ankitsultana (via GitHub)" <gi...@apache.org>.
ankitsultana commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145180705
##########
pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingTable.java:
##########
@@ -25,16 +25,37 @@
public class RoutingTable {
private final Map<ServerInstance, List<String>> _serverInstanceToSegmentsMap;
+ private final Map<ServerInstance, Map<Integer, List<String>>> _serverInstanceToPartitionedSegmentsMap;
private final List<String> _unavailableSegments;
private final int _numPrunedSegments;
+ private final boolean _isPartitionRouting;
- public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap, List<String> unavailableSegments,
- int numPrunedSegments) {
+ public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap,
+ List<String> unavailableSegments, int numPrunedSegments) {
_serverInstanceToSegmentsMap = serverInstanceToSegmentsMap;
+ _serverInstanceToPartitionedSegmentsMap = null;
+ _isPartitionRouting = false;
_unavailableSegments = unavailableSegments;
_numPrunedSegments = numPrunedSegments;
}
+ public RoutingTable(Map<ServerInstance, Map<Integer, List<String>>> serverInstanceToPartitionedSegmentsMap,
+ List<String> unavailableSegments, int numPrunedSegments, boolean isPartitionRouting) {
+ _serverInstanceToPartitionedSegmentsMap = serverInstanceToPartitionedSegmentsMap;
+ _serverInstanceToSegmentsMap = null;
+ _isPartitionRouting = isPartitionRouting;
+ _unavailableSegments = unavailableSegments;
+ _numPrunedSegments = numPrunedSegments;
+ }
+
+ public boolean isPartitionRouting() {
+ return _isPartitionRouting;
+ }
+
+ public Map<ServerInstance, Map<Integer, List<String>>> getServerInstanceToPartitionedSegmentsMap() {
Review Comment:
`@Nullable`.
##########
pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingTable.java:
##########
@@ -25,16 +25,37 @@
public class RoutingTable {
private final Map<ServerInstance, List<String>> _serverInstanceToSegmentsMap;
+ private final Map<ServerInstance, Map<Integer, List<String>>> _serverInstanceToPartitionedSegmentsMap;
private final List<String> _unavailableSegments;
private final int _numPrunedSegments;
+ private final boolean _isPartitionRouting;
- public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap, List<String> unavailableSegments,
- int numPrunedSegments) {
+ public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap,
Review Comment:
Should we add a `PartitionRoutingTable` which extends this? One advantage of that is we can have added type-safety to ensure the right version of the routing table is passed around.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,105 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+
+
+/**
+ *
Review Comment:
javadoc
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -105,6 +106,8 @@ public class BrokerRoutingManager implements RoutingManager, ClusterChangeHandle
private String _idealStatePathPrefix;
private String _instanceConfigsPath;
private ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ // TODO: Entries are never removed from map
+ private Map<String, SegmentZkMetadataCache> _segmentMetadataCacheMap;
Review Comment:
I can't see where we are adding entries to this map. Is this map needed?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1152049647
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
sounds good. will follow up with the refactoring
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
+ private final Set<String> _onlineSegmentsCached;
+
+ public SegmentZkMetadataCache(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore,
+ List<SegmentPruner> segmentPruners) {
+ _tableNameWithType = tableNameWithType;
+ _propertyStore = propertyStore;
+ _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(tableNameWithType) + "/";
+ _registeredPruners = segmentPruners;
+ _onlineSegmentsCached = new HashSet<>();
+ }
+
+ public void init(IdealState idealState, ExternalView externalView, Set<String> onlineSegments) {
+ if (_registeredPruners.size() > 0) {
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1152611450
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -722,7 +723,7 @@ InstanceSelector.SelectionResult calculateRouting(BrokerRequest brokerRequest, l
Set<String> selectedSegments = _segmentSelector.select(brokerRequest);
int numTotalSelectedSegments = selectedSegments.size();
if (!selectedSegments.isEmpty()) {
- for (SegmentPruner segmentPruner : _segmentPruners) {
+ for (SegmentPruner segmentPruner : _segmentMetadataCache.getPruners()) {
Review Comment:
We should keep both pruners and metadata fetcher in the routing entry. Fetcher shouldn't have any knowledge on the pruner
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -635,9 +639,9 @@ private static class RoutingEntry {
final String _externalViewPath;
final SegmentPreSelector _segmentPreSelector;
final SegmentSelector _segmentSelector;
- final List<SegmentPruner> _segmentPruners;
final InstanceSelector _instanceSelector;
final Long _queryTimeoutMs;
+ final SegmentZkMetadataFetcher _segmentMetadataCache;
Review Comment:
(minor) Rename the variable
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
+ private final Set<String> _onlineSegmentsCached;
+
+ public SegmentZkMetadataFetcher(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore,
+ List<SegmentPruner> segmentPruners) {
Review Comment:
Metadata fetcher shouldn't have any knowledge on the segment pruners.
We should add a `void register(Listener listener)` method, which is used to register the listeners before calling the `init()`
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145487118
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
yeah maybe the naming is not as intuitive. the problem @Jackie-Jiang mentioned is that caching segment zk metadata as a whole will create a large memory footprint. so this class actually only caches the online segment names. and invoke `onAssignmentChange` and `refreshSegment` of the registered methods by batch calling property store to get ZNRecords
in the next PR I am going to
- make TimeBoundaryManager also registered with SegmentZkMetadataCache so it doesn't need to individually pull from ZK
- create a PartitionManager to manage segment-partition/server mapping info very similar to SinglePartitionPruner
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1152681020
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
+ private final Set<String> _onlineSegmentsCached;
+
+ public SegmentZkMetadataFetcher(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore,
+ List<SegmentPruner> segmentPruners) {
Review Comment:
currently we have to register all at the same method so having register method is the same as passing in the list during construction. but reworked and added the API
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10455: create segment zk metadata cache
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10455:
URL: https://github.com/apache/pinot/pull/10455#issuecomment-1478733353
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10455](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0b38105) into [master](https://codecov.io/gh/apache/pinot/commit/cb1069f11d8b89291e777c6e2c7416753bbfff29?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (cb1069f) will **decrease** coverage by `56.33%`.
> The diff coverage is `82.82%`.
```diff
@@ Coverage Diff @@
## master #10455 +/- ##
=============================================
- Coverage 70.24% 13.91% -56.33%
+ Complexity 6110 237 -5873
=============================================
Files 2061 2014 -47
Lines 111752 109377 -2375
Branches 16985 16703 -282
=============================================
- Hits 78504 15224 -63280
- Misses 27720 92923 +65203
+ Partials 5528 1230 -4298
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `13.91% <82.82%> (-0.02%)` | :arrow_down: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...mentpruner/MultiPartitionColumnsSegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL011bHRpUGFydGl0aW9uQ29sdW1uc1NlZ21lbnRQcnVuZXIuamF2YQ==) | `71.96% <75.00%> (-1.96%)` | :arrow_down: |
| [...oker/routing/segmentpruner/EmptySegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL0VtcHR5U2VnbWVudFBydW5lci5qYXZh) | `85.41% <83.33%> (-2.52%)` | :arrow_down: |
| [...che/pinot/broker/routing/BrokerRoutingManager.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9Ccm9rZXJSb3V0aW5nTWFuYWdlci5qYXZh) | `61.69% <85.71%> (-24.11%)` | :arrow_down: |
| [...mentpruner/SinglePartitionColumnSegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1NpbmdsZVBhcnRpdGlvbkNvbHVtblNlZ21lbnRQcnVuZXIuamF2YQ==) | `64.04% <85.71%> (-8.41%)` | :arrow_down: |
| [...outing/segmentmetadata/SegmentZkMetadataCache.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50bWV0YWRhdGEvU2VnbWVudFprTWV0YWRhdGFDYWNoZS5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...er/routing/segmentpruner/SegmentPrunerFactory.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1NlZ21lbnRQcnVuZXJGYWN0b3J5LmphdmE=) | `93.58% <100.00%> (+0.83%)` | :arrow_up: |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/pinot/pull/10455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `72.51% <100.00%> (-1.72%)` | :arrow_down: |
... and [1616 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10455/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145361912
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
Feeling we shouldn't couple SegmentZkMetadataCache with Pruner here. This SegmentZkMetadataCache can be used else where.
SegmentZkMetadataCache should be more like a util singleton. Then all pruners can use it.
The behavior should be similar to the TableCache for table config and schema.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145487118
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
yeah maybe the naming is not as intuitive. the problem @Jackie-Jiang mentioned is that caching segment zk metadata as a whole will create a large memory footprint. so this class actually only caches the online segment names. and invoke `onAssignmentChange` and `refreshSegment` of the registered methods.
in the next PR I am going to
- make TimeBoundaryManager also registered with SegmentZkMetadataCache so it doesn't need to individually pull from ZK
- create a PartitionManager to manage segment-partition/server mapping info very similar to SinglePartitionPruner
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1152695449
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
chatted with Jackie offline. we decided to introduce the interface in this PR. so added `SegmentZkMetadataFetchListener`
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145193532
##########
pinot-core/src/main/java/org/apache/pinot/core/routing/RoutingTable.java:
##########
@@ -25,16 +25,37 @@
public class RoutingTable {
private final Map<ServerInstance, List<String>> _serverInstanceToSegmentsMap;
+ private final Map<ServerInstance, Map<Integer, List<String>>> _serverInstanceToPartitionedSegmentsMap;
private final List<String> _unavailableSegments;
private final int _numPrunedSegments;
+ private final boolean _isPartitionRouting;
- public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap, List<String> unavailableSegments,
- int numPrunedSegments) {
+ public RoutingTable(Map<ServerInstance, List<String>> serverInstanceToSegmentsMap,
+ List<String> unavailableSegments, int numPrunedSegments) {
_serverInstanceToSegmentsMap = serverInstanceToSegmentsMap;
+ _serverInstanceToPartitionedSegmentsMap = null;
+ _isPartitionRouting = false;
_unavailableSegments = unavailableSegments;
_numPrunedSegments = numPrunedSegments;
}
+ public RoutingTable(Map<ServerInstance, Map<Integer, List<String>>> serverInstanceToPartitionedSegmentsMap,
+ List<String> unavailableSegments, int numPrunedSegments, boolean isPartitionRouting) {
+ _serverInstanceToPartitionedSegmentsMap = serverInstanceToPartitionedSegmentsMap;
+ _serverInstanceToSegmentsMap = null;
+ _isPartitionRouting = isPartitionRouting;
+ _unavailableSegments = unavailableSegments;
+ _numPrunedSegments = numPrunedSegments;
+ }
+
+ public boolean isPartitionRouting() {
+ return _isPartitionRouting;
+ }
+
+ public Map<ServerInstance, Map<Integer, List<String>>> getServerInstanceToPartitionedSegmentsMap() {
Review Comment:
this change should not be in this PR. will revert and add in separate 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-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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1151165950
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
Review Comment:
(minor) It serves as a fetcher instead of a cache. It is used to ensure the ZK metadata is fetched once per segment
```suggestion
public class SegmentZKMetadataFetcher {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
+1. We should introduce an interface for the 3 APIs needed to manage the ZK metadata. `SegmentPruner` can extend this new interface, with an additional `prune()` method. This way, `TimeBoundaryManager` can implement this new interface
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
+ private final Set<String> _onlineSegmentsCached;
+
+ public SegmentZkMetadataCache(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore,
+ List<SegmentPruner> segmentPruners) {
+ _tableNameWithType = tableNameWithType;
+ _propertyStore = propertyStore;
+ _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(tableNameWithType) + "/";
+ _registeredPruners = segmentPruners;
+ _onlineSegmentsCached = new HashSet<>();
+ }
+
+ public void init(IdealState idealState, ExternalView externalView, Set<String> onlineSegments) {
+ if (_registeredPruners.size() > 0) {
Review Comment:
(nit) same for others
```suggestion
if (!_registeredPruners.isEmpty()) {
```
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145672147
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataCache.java:
##########
@@ -0,0 +1,107 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.broker.routing.segmentpruner.SegmentPruner;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+import org.apache.pinot.common.request.BrokerRequest;
+
+
+/**
+ * {@code SegmentZkMetadataCache} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments. This is used by {@link SegmentPruner#prune(BrokerRequest, Set)}.
+ */
+public class SegmentZkMetadataCache {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentPruner> _registeredPruners;
Review Comment:
see follow up draft in https://github.com/apache/pinot/pull/10461. it is not yet finalized.
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1145193210
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/BrokerRoutingManager.java:
##########
@@ -105,6 +106,8 @@ public class BrokerRoutingManager implements RoutingManager, ClusterChangeHandle
private String _idealStatePathPrefix;
private String _instanceConfigsPath;
private ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ // TODO: Entries are never removed from map
+ private Map<String, SegmentZkMetadataCache> _segmentMetadataCacheMap;
Review Comment:
good catch. i originally put it in here but decided to merge it with routingEntry. this is no longer needed.
--
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
[GitHub] [pinot] Jackie-Jiang commented on a diff in pull request #10455: create segment zk metadata cache
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10455:
URL: https://github.com/apache/pinot/pull/10455#discussion_r1153795246
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentZkMetadataFetchListener> _registeredListeners;
+ private final Set<String> _onlineSegmentsCached;
+
+ private boolean _initialized;
+
+ public SegmentZkMetadataFetcher(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _tableNameWithType = tableNameWithType;
+ _propertyStore = propertyStore;
+ _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(tableNameWithType) + "/";
+ _registeredListeners = new ArrayList<>();
+ _onlineSegmentsCached = new HashSet<>();
+ _initialized = false;
+ }
+
+ public void init(IdealState idealState, ExternalView externalView, Set<String> onlineSegments) {
+ if (!_initialized) {
+ _initialized = true;
+ if (!_registeredListeners.isEmpty()) {
+ // Bulk load partition info for all online segments
+ int numSegments = onlineSegments.size();
+ List<String> segments = new ArrayList<>(numSegments);
+ List<String> segmentZKMetadataPaths = new ArrayList<>(numSegments);
+ for (String segment : onlineSegments) {
+ segments.add(segment);
+ segmentZKMetadataPaths.add(_segmentZKMetadataPathPrefix + segment);
+ }
+ _onlineSegmentsCached.addAll(onlineSegments);
+ List<ZNRecord> znRecords = _propertyStore.get(segmentZKMetadataPaths, null, AccessOption.PERSISTENT, false);
+ for (SegmentZkMetadataFetchListener listener : _registeredListeners) {
+ listener.init(idealState, externalView, segments, znRecords);
+ }
+ }
+ } else {
+ throw new RuntimeException("Segment zk metadata fetcher has already been initialized!");
+ }
+ }
+
+ public void registerListener(SegmentZkMetadataFetchListener listener) {
+ if (!_initialized) {
+ _registeredListeners.add(listener);
+ } else {
+ throw new RuntimeException("Segment zk metadata fetcher has already been initialized! "
+ + "Unable to register more listeners.");
+ }
+ }
+
+ public List<SegmentZkMetadataFetchListener> getRegisteredListeners() {
Review Comment:
(minor)
```suggestion
public List<SegmentZkMetadataFetchListener> getListeners() {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentZkMetadataFetchListener> _registeredListeners;
Review Comment:
(minor)
```suggestion
private final List<SegmentZkMetadataFetchListener> _listeners;
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentZkMetadataFetchListener> _registeredListeners;
+ private final Set<String> _onlineSegmentsCached;
+
+ private boolean _initialized;
+
+ public SegmentZkMetadataFetcher(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _tableNameWithType = tableNameWithType;
+ _propertyStore = propertyStore;
+ _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(tableNameWithType) + "/";
+ _registeredListeners = new ArrayList<>();
+ _onlineSegmentsCached = new HashSet<>();
+ _initialized = false;
+ }
+
+ public void init(IdealState idealState, ExternalView externalView, Set<String> onlineSegments) {
+ if (!_initialized) {
+ _initialized = true;
+ if (!_registeredListeners.isEmpty()) {
+ // Bulk load partition info for all online segments
+ int numSegments = onlineSegments.size();
+ List<String> segments = new ArrayList<>(numSegments);
+ List<String> segmentZKMetadataPaths = new ArrayList<>(numSegments);
+ for (String segment : onlineSegments) {
+ segments.add(segment);
+ segmentZKMetadataPaths.add(_segmentZKMetadataPathPrefix + segment);
+ }
+ _onlineSegmentsCached.addAll(onlineSegments);
+ List<ZNRecord> znRecords = _propertyStore.get(segmentZKMetadataPaths, null, AccessOption.PERSISTENT, false);
+ for (SegmentZkMetadataFetchListener listener : _registeredListeners) {
+ listener.init(idealState, externalView, segments, znRecords);
+ }
+ }
+ } else {
+ throw new RuntimeException("Segment zk metadata fetcher has already been initialized!");
+ }
+ }
+
+ public void registerListener(SegmentZkMetadataFetchListener listener) {
Review Comment:
(minor)
```suggestion
public void register(SegmentZkMetadataFetchListener listener) {
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/segmentmetadata/SegmentZkMetadataFetcher.java:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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.broker.routing.segmentmetadata;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.AccessOption;
+import org.apache.helix.model.ExternalView;
+import org.apache.helix.model.IdealState;
+import org.apache.helix.store.zk.ZkHelixPropertyStore;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.common.metadata.ZKMetadataProvider;
+
+
+/**
+ * {@code SegmentZkMetadataFetcher} is used to cache {@link ZNRecord} stored in {@link ZkHelixPropertyStore} for
+ * segments.
+ */
+public class SegmentZkMetadataFetcher {
+ private final String _tableNameWithType;
+ private final ZkHelixPropertyStore<ZNRecord> _propertyStore;
+ private final String _segmentZKMetadataPathPrefix;
+ private final List<SegmentZkMetadataFetchListener> _registeredListeners;
+ private final Set<String> _onlineSegmentsCached;
+
+ private boolean _initialized;
+
+ public SegmentZkMetadataFetcher(String tableNameWithType, ZkHelixPropertyStore<ZNRecord> propertyStore) {
+ _tableNameWithType = tableNameWithType;
+ _propertyStore = propertyStore;
+ _segmentZKMetadataPathPrefix = ZKMetadataProvider.constructPropertyStorePathForResource(tableNameWithType) + "/";
+ _registeredListeners = new ArrayList<>();
+ _onlineSegmentsCached = new HashSet<>();
+ _initialized = false;
+ }
+
+ public void init(IdealState idealState, ExternalView externalView, Set<String> onlineSegments) {
+ if (!_initialized) {
+ _initialized = true;
+ if (!_registeredListeners.isEmpty()) {
+ // Bulk load partition info for all online segments
+ int numSegments = onlineSegments.size();
+ List<String> segments = new ArrayList<>(numSegments);
+ List<String> segmentZKMetadataPaths = new ArrayList<>(numSegments);
+ for (String segment : onlineSegments) {
+ segments.add(segment);
+ segmentZKMetadataPaths.add(_segmentZKMetadataPathPrefix + segment);
+ }
+ _onlineSegmentsCached.addAll(onlineSegments);
+ List<ZNRecord> znRecords = _propertyStore.get(segmentZKMetadataPaths, null, AccessOption.PERSISTENT, false);
+ for (SegmentZkMetadataFetchListener listener : _registeredListeners) {
+ listener.init(idealState, externalView, segments, znRecords);
+ }
+ }
+ } else {
+ throw new RuntimeException("Segment zk metadata fetcher has already been initialized!");
+ }
+ }
+
+ public void registerListener(SegmentZkMetadataFetchListener listener) {
+ if (!_initialized) {
+ _registeredListeners.add(listener);
+ } else {
+ throw new RuntimeException("Segment zk metadata fetcher has already been initialized! "
+ + "Unable to register more listeners.");
+ }
+ }
+
+ public List<SegmentZkMetadataFetchListener> getRegisteredListeners() {
+ return _registeredListeners;
+ }
+
+ public synchronized void onAssignmentChange(IdealState idealState, ExternalView externalView,
+ Set<String> onlineSegments) {
+ if (!_registeredListeners.isEmpty()) {
+ int numSegments = onlineSegments.size();
+ List<String> segments = new ArrayList<>(numSegments);
+ List<String> segmentZKMetadataPaths = new ArrayList<>(numSegments);
+ for (String segment : onlineSegments) {
+ if (!_onlineSegmentsCached.contains(segment)) {
+ segments.add(segment);
+ segmentZKMetadataPaths.add(_segmentZKMetadataPathPrefix + segment);
+ }
+ }
+ _onlineSegmentsCached.addAll(onlineSegments);
Review Comment:
(minor) Can be simplified
```suggestion
for (String segment : onlineSegments) {
if (_onlineSegmentsCached.add(segment)) {
segments.add(segment);
segmentZKMetadataPaths.add(_segmentZKMetadataPathPrefix + segment);
}
}
```
--
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