You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/04/11 17:21:15 UTC
[GitHub] [pinot] dongxiaoman opened a new pull request, #8506: Upgrade helix to 0.9.10
dongxiaoman opened a new pull request, #8506:
URL: https://github.com/apache/pinot/pull/8506
== Description
Upgrade helix dependency to `0.9.10` for fixing the issues related to ZNode size limit.
1. Stripe has tested out helix `0.9.9` on private build and surpassed the ZNode limit. (David Yang at Stripe)
The Helix `0.9.10` is basically the same as `0.9.9` based on git diff history. [The 0.9.9 release note link is here.](https://helix.apache.org/0.9.9-docs/releasenotes/release-0.9.9.html)
== Release Notes
Upgraded helix dependency to `0.9.10` for fixing the issues caused by ZNode size limit.
--
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] dongxiaoman commented on pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1097077899
@mcvsubbu @jackjlli (Let me update that part in the description)
1. The fix in Helix `0.9.9` unblocks the case when the ZNode is larger than 1MB **even after compression**. The [code change in 0.9.9 is here](https://github.com/apache/helix/blame/5acf3e14c96281e3d83323f2b630efa4f474458a/helix-core/src/main/java/org/apache/helix/manager/zk/zookeeper/ZkClient.java#L89) while the limit is applied when we [update Zookeeper nodes from the same ZkClient](https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/apache/helix/manager/zk/zookeeper/ZkClient.java#L1416). It happens when the Pinot table has lots of segments with long hostnames, for IdealStates node.
2. Right now it will be awesome to have Helix 1.0+ in; if it is not catching the next release, it will be good for the community to upgrade `0.9.9` since this Helix back-port patch is actually requested by us Pinot
For @Jackie-Jiang I will roll back to `0.9.9` ; mostly I found the `0.9.10` in [Maven Repo](https://mvnrepository.com/artifact/org.apache.helix/helix-core/0.9.10) so I thought it is released
Let me also design an integration test case where the ZNode bug will show up.
--
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 #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1095353201
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8506?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 [#8506](https://codecov.io/gh/apache/pinot/pull/8506?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e49272) into [master](https://codecov.io/gh/apache/pinot/commit/fd3eceaea0d9dd1fe3581fb235e1c389340f79cb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (fd3ecea) will **decrease** coverage by `11.71%`.
> The diff coverage is `n/a`.
> :exclamation: Current head 3e49272 differs from pull request most recent head dc6c7dd. Consider uploading reports for the commit dc6c7dd to get more accurate results
```diff
@@ Coverage Diff @@
## master #8506 +/- ##
=============================================
- Coverage 25.83% 14.11% -11.72%
- Complexity 0 84 +84
=============================================
Files 1660 1627 -33
Lines 87129 85576 -1553
Branches 13203 13037 -166
=============================================
- Hits 22513 12083 -10430
- Misses 62459 72584 +10125
+ Partials 2157 909 -1248
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `?` | |
| unittests2 | `14.11% <ø> (?)` | |
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/8506?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?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: |
| [...a/org/apache/pinot/core/operator/BaseOperator.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9CYXNlT3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/core/plan/GlobalPlanImplV0.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0dsb2JhbFBsYW5JbXBsVjAuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/core/plan/SelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/core/plan/TransformPlanNode.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1RyYW5zZm9ybVBsYW5Ob2RlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/BrokerMeter.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Ccm9rZXJNZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [778 more](https://codecov.io/gh/apache/pinot/pull/8506/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8506?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8506?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [fd3ecea...dc6c7dd](https://codecov.io/gh/apache/pinot/pull/8506?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] dongxiaoman commented on a diff in pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on code in PR #8506:
URL: https://github.com/apache/pinot/pull/8506#discussion_r850928592
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HelixZNodeSizeLimitTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.integration.tests;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * This test is created to show the bug in Helix 0.9.8 that if a ZooKeeper IdealState is larger than 1MB after
+ * compression, it cannot be updated anymore. Somehow this test can also make sure in future we will support
+ * large IdealStates
+ */
+public class HelixZNodeSizeLimitTest extends BaseClusterIntegrationTest {
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ // This line of code has to be executed before the org.apache.helix.manager.zk.zookeeper.ZkClient.WRITE_SIZE_LIMIT
+ // is initialized. The code is in
+ // https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/
+ // apache/helix/manager/zk/zookeeper/ZkClient.java#L89
+ // Not sure how this works but this below line executes before ZkClient.WRITE_SIZE_LIMIT is created
+ System.setProperty(SystemPropertyKeys.JUTE_MAXBUFFER, "4000000");
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir);
+
+ // Start Zookeeper
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ addSchema(createSchema());
+ addTableConfig(createOfflineTableConfig());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Test
+ public void testUpdateIdealState() {
+ // In Helix 0.9.8, we get error logs like below:
+ // 13:03:51.576 ERROR [ZkBaseDataAccessor] [main] Exception while setting path:
+ // /HelixZNodeSizeLimitTest/IDEALSTATES/mytable_OFFLINE
+ // org.apache.helix.HelixException: Data size larger than 1M
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.checkDataSizeLimit(ZkClient.java:1513) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.writeDataReturnStat(ZkClient.java:1406) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ String tableNameWithType = getTableName() + "_OFFLINE";
+ System.setProperty(SystemPropertyKeys.ZK_SERIALIZER_ZNRECORD_WRITE_SIZE_LIMIT_BYTES, "4000000");
+ // The updated IdealState after compression is roughly 2MB
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType, idealState -> {
Review Comment:
@jackjlli Thanks for the verification!
For your question: do we have a rough estimation of when Helix 1.0+ will be validated? like in 1~2 months? Right now there's no blocker for us but I think it really depends on how long do we need to wait.
And secondly, I think it is worth keeping that extra unit test; feel free to copy the code into your PR if needed, or I can convert this PR into "unit test only" after yours is landed.
--
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 pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1096978168
Since `0.9.9` and `0.9.10` are the same, why do we upgrade to `0.9.10` instead of `0.9.9`? I don't see `0.9.10` as an official release in Helix
--
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] dongxiaoman commented on pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1095331058
(cannot add labels in the UI, and somehow cannot find how to markdown a label)
--
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] dongxiaoman closed pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman closed pull request #8506: Upgrade helix to 0.9.10
URL: https://github.com/apache/pinot/pull/8506
--
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] jackjlli commented on a diff in pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8506:
URL: https://github.com/apache/pinot/pull/8506#discussion_r850920845
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HelixZNodeSizeLimitTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.integration.tests;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * This test is created to show the bug in Helix 0.9.8 that if a ZooKeeper IdealState is larger than 1MB after
+ * compression, it cannot be updated anymore. Somehow this test can also make sure in future we will support
+ * large IdealStates
+ */
+public class HelixZNodeSizeLimitTest extends BaseClusterIntegrationTest {
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ // This line of code has to be executed before the org.apache.helix.manager.zk.zookeeper.ZkClient.WRITE_SIZE_LIMIT
+ // is initialized. The code is in
+ // https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/
+ // apache/helix/manager/zk/zookeeper/ZkClient.java#L89
+ // Not sure how this works but this below line executes before ZkClient.WRITE_SIZE_LIMIT is created
+ System.setProperty(SystemPropertyKeys.JUTE_MAXBUFFER, "4000000");
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir);
+
+ // Start Zookeeper
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ addSchema(createSchema());
+ addTableConfig(createOfflineTableConfig());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Test
+ public void testUpdateIdealState() {
+ // In Helix 0.9.8, we get error logs like below:
+ // 13:03:51.576 ERROR [ZkBaseDataAccessor] [main] Exception while setting path:
+ // /HelixZNodeSizeLimitTest/IDEALSTATES/mytable_OFFLINE
+ // org.apache.helix.HelixException: Data size larger than 1M
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.checkDataSizeLimit(ZkClient.java:1513) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.writeDataReturnStat(ZkClient.java:1406) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ String tableNameWithType = getTableName() + "_OFFLINE";
+ System.setProperty(SystemPropertyKeys.ZK_SERIALIZER_ZNRECORD_WRITE_SIZE_LIMIT_BYTES, "4000000");
+ // The updated IdealState after compression is roughly 2MB
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType, idealState -> {
Review Comment:
Thanks for making the changes here! I've verified that the new code is also included in Helix 1.0+ release. Once Helix 1.0+ is merged (which will happen pretty soon), I don't think we need this PR any more. Why is it so urgent to merge? If it's not so urgent, I'd suggest waiting for the Helix 1.0+. Otherwise, it will still need some time and effort to validate this 0.9.10 release.
--
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] dongxiaoman commented on pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1097204320
> Since `0.9.9` and `0.9.10` are the same, why do we upgrade to `0.9.10` instead of `0.9.9`? I don't see `0.9.10` as an official release in Helix
After trying in IntelliJ with source, there are some discrepancy between the Helix source code tags and the actual artifacts release.
Simply put, the folks in Helix made a mistake in the released artifact of `0.9.9` build, and the `0.9.10` is the actual one containing fixes.
--
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] jackjlli commented on a diff in pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8506:
URL: https://github.com/apache/pinot/pull/8506#discussion_r851407420
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HelixZNodeSizeLimitTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.integration.tests;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * This test is created to show the bug in Helix 0.9.8 that if a ZooKeeper IdealState is larger than 1MB after
+ * compression, it cannot be updated anymore. Somehow this test can also make sure in future we will support
+ * large IdealStates
+ */
+public class HelixZNodeSizeLimitTest extends BaseClusterIntegrationTest {
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ // This line of code has to be executed before the org.apache.helix.manager.zk.zookeeper.ZkClient.WRITE_SIZE_LIMIT
+ // is initialized. The code is in
+ // https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/
+ // apache/helix/manager/zk/zookeeper/ZkClient.java#L89
+ // Not sure how this works but this below line executes before ZkClient.WRITE_SIZE_LIMIT is created
+ System.setProperty(SystemPropertyKeys.JUTE_MAXBUFFER, "4000000");
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir);
+
+ // Start Zookeeper
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ addSchema(createSchema());
+ addTableConfig(createOfflineTableConfig());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Test
+ public void testUpdateIdealState() {
+ // In Helix 0.9.8, we get error logs like below:
+ // 13:03:51.576 ERROR [ZkBaseDataAccessor] [main] Exception while setting path:
+ // /HelixZNodeSizeLimitTest/IDEALSTATES/mytable_OFFLINE
+ // org.apache.helix.HelixException: Data size larger than 1M
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.checkDataSizeLimit(ZkClient.java:1513) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.writeDataReturnStat(ZkClient.java:1406) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ String tableNameWithType = getTableName() + "_OFFLINE";
+ System.setProperty(SystemPropertyKeys.ZK_SERIALIZER_ZNRECORD_WRITE_SIZE_LIMIT_BYTES, "4000000");
+ // The updated IdealState after compression is roughly 2MB
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType, idealState -> {
Review Comment:
@dongxiaoman Added the unit test into the other PR (some classes have been renamed thus adjusted some of your comments there):
https://github.com/apache/pinot/pull/8325/files#diff-5ed6b0dd4c47b74179789062c2413dda3b74f2640248037e159d2c3981c38b8c
--
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] dongxiaoman commented on pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1130250606
close this one to clean up since https://github.com/apache/pinot/pull/8325/files#diff-5ed6b0dd4c47b74179789062c2413dda3b74f2640248037e159d2c3981c38b8c is in place already
--
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] jackjlli commented on a diff in pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8506:
URL: https://github.com/apache/pinot/pull/8506#discussion_r851392902
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HelixZNodeSizeLimitTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.integration.tests;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * This test is created to show the bug in Helix 0.9.8 that if a ZooKeeper IdealState is larger than 1MB after
+ * compression, it cannot be updated anymore. Somehow this test can also make sure in future we will support
+ * large IdealStates
+ */
+public class HelixZNodeSizeLimitTest extends BaseClusterIntegrationTest {
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ // This line of code has to be executed before the org.apache.helix.manager.zk.zookeeper.ZkClient.WRITE_SIZE_LIMIT
+ // is initialized. The code is in
+ // https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/
+ // apache/helix/manager/zk/zookeeper/ZkClient.java#L89
+ // Not sure how this works but this below line executes before ZkClient.WRITE_SIZE_LIMIT is created
+ System.setProperty(SystemPropertyKeys.JUTE_MAXBUFFER, "4000000");
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir);
+
+ // Start Zookeeper
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ addSchema(createSchema());
+ addTableConfig(createOfflineTableConfig());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Test
+ public void testUpdateIdealState() {
+ // In Helix 0.9.8, we get error logs like below:
+ // 13:03:51.576 ERROR [ZkBaseDataAccessor] [main] Exception while setting path:
+ // /HelixZNodeSizeLimitTest/IDEALSTATES/mytable_OFFLINE
+ // org.apache.helix.HelixException: Data size larger than 1M
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.checkDataSizeLimit(ZkClient.java:1513) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.writeDataReturnStat(ZkClient.java:1406) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ String tableNameWithType = getTableName() + "_OFFLINE";
+ System.setProperty(SystemPropertyKeys.ZK_SERIALIZER_ZNRECORD_WRITE_SIZE_LIMIT_BYTES, "4000000");
+ // The updated IdealState after compression is roughly 2MB
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType, idealState -> {
Review Comment:
@dongxiaoman Yes, the integration of Helix 1.0+ will be done in this quarter, roughly 1~2 months. The PR will be ready only after it's fully tested and deployed in LinkedIn. And I agree that we should keep that unit test into the code. I'll put it to the same PR. Thanks again!
--
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] dongxiaoman commented on a diff in pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
dongxiaoman commented on code in PR #8506:
URL: https://github.com/apache/pinot/pull/8506#discussion_r849629790
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/HelixZNodeSizeLimitTest.java:
##########
@@ -0,0 +1,91 @@
+/**
+ * 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.integration.tests;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.Map;
+import org.apache.helix.SystemPropertyKeys;
+import org.apache.pinot.common.utils.helix.HelixHelper;
+import org.apache.pinot.util.TestUtils;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * This test is created to show the bug in Helix 0.9.8 that if a ZooKeeper IdealState is larger than 1MB after
+ * compression, it cannot be updated anymore. Somehow this test can also make sure in future we will support
+ * large IdealStates
+ */
+public class HelixZNodeSizeLimitTest extends BaseClusterIntegrationTest {
+ @BeforeClass
+ public void setUp()
+ throws Exception {
+ // This line of code has to be executed before the org.apache.helix.manager.zk.zookeeper.ZkClient.WRITE_SIZE_LIMIT
+ // is initialized. The code is in
+ // https://github.com/apache/helix/blob/helix-0.9.9/helix-core/src/main/java/org/
+ // apache/helix/manager/zk/zookeeper/ZkClient.java#L89
+ // Not sure how this works but this below line executes before ZkClient.WRITE_SIZE_LIMIT is created
+ System.setProperty(SystemPropertyKeys.JUTE_MAXBUFFER, "4000000");
+ TestUtils.ensureDirectoriesExistAndEmpty(_tempDir);
+
+ // Start Zookeeper
+ startZk();
+ startController();
+ startBroker();
+ startServer();
+ addSchema(createSchema());
+ addTableConfig(createOfflineTableConfig());
+ }
+
+ @AfterClass(alwaysRun = true)
+ public void tearDown()
+ throws Exception {
+ stopServer();
+ stopBroker();
+ stopController();
+ stopZk();
+ }
+
+ @Test
+ public void testUpdateIdealState() {
+ // In Helix 0.9.8, we get error logs like below:
+ // 13:03:51.576 ERROR [ZkBaseDataAccessor] [main] Exception while setting path:
+ // /HelixZNodeSizeLimitTest/IDEALSTATES/mytable_OFFLINE
+ // org.apache.helix.HelixException: Data size larger than 1M
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.checkDataSizeLimit(ZkClient.java:1513) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ // at org.apache.helix.manager.zk.zookeeper.ZkClient.writeDataReturnStat(ZkClient.java:1406) ~[helix-core-0.9.8
+ // .jar:0.9.8]
+ String tableNameWithType = getTableName() + "_OFFLINE";
+ System.setProperty(SystemPropertyKeys.ZK_SERIALIZER_ZNRECORD_WRITE_SIZE_LIMIT_BYTES, "4000000");
+ // The updated IdealState after compression is roughly 2MB
+ HelixHelper.updateIdealState(_helixManager, tableNameWithType, idealState -> {
Review Comment:
@mcvsubbu @jackjlli This test case is what the Helix upgrade is for. The IdealState after compression is more than 2MB, and Helix will refuse to write 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-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] mcvsubbu commented on pull request #8506: Upgrade helix to 0.9.10
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on PR #8506:
URL: https://github.com/apache/pinot/pull/8506#issuecomment-1096983582
@dongxiaoman I agree with @jackjlli . We do have an impending change to Helix 1.x. Helix version upgrades in the past have required a good bit of testing, and we are going through it in a thorough manner in the 1.x case. I would recommend to wait until then, and use the current methods available to enable compression, etc.
What is the use case you are trying to solve? Is it idealstate getting to be too big? currentstate? We can share our helix configs if you like. We have a cluster with 4k+ tables of all shapes and sizes.
--
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