You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/11/25 09:39:00 UTC
[GitHub] [pulsar] Jason918 opened a new pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Jason918 opened a new pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975
Fixes #12812
### Motivation
See #12812
### Modifications
Add AbstractBatchedMetadataStore and ZKBatchedMetadataStore.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is already covered by existing tests, all test cases in pulsar-metadata.
This change added tests and can be verified as follows:
- ZKBatchedMetadataStoreTest
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
Pref optimization.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#discussion_r758894264
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKBatchedMetadataStore.java
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.pulsar.metadata.impl;
+
+
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pulsar.metadata.api.GetResult;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.impl.batch.BatchWorker;
+import org.apache.pulsar.metadata.impl.batch.MetadataOp;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+
+@Slf4j
+public class ZKBatchedMetadataStore extends ZKMetadataStore implements AsyncCallback.MultiCallback {
Review comment:
I wrote a AbstractBatchedMetadataStore at first, but got some issue.
- We have to use two different batching queues for Zookeeper Multi operations, one for read and one for write. But I believe that we don't need to do that in other metadata store implementations, e.g. Memory, RocksDB or maybe etcd version of implementation.
- There are not many things left in this AbstractBatchedMetadataStore, mostly is in wrapped in `BatchWorker`, maybe only `fallbackToSingleOps`.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-979842818
/pulsarbot run-failure-checks
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-981098704
/pulsarbot run-failure-checks
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-982379554
@Jason918 I've added a different PR based on the approach I was mentioning: #13043. PTAL
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-980826119
/pulsarbot run-failure-checks
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#discussion_r758894798
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKBatchedMetadataStore.java
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.pulsar.metadata.impl;
+
+
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pulsar.metadata.api.GetResult;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.impl.batch.BatchWorker;
+import org.apache.pulsar.metadata.impl.batch.MetadataOp;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+
+@Slf4j
+public class ZKBatchedMetadataStore extends ZKMetadataStore implements AsyncCallback.MultiCallback {
+
+ protected final boolean metadataStoreBatchingEnable = true;
+ protected final int metadataStoreBatchingMaxDelayMillis = 5;
+ protected final int metadataStoreBatchingMaxOperations = 100;
+ protected final int metadataStoreBatchingMaxSizeKb = 128;
Review comment:
OK, I will fix this.
@codelipenghui @hangc0276 FYI.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 closed pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 closed pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-982224750
> This is implementing the batching for `getData`/`getChildren` calls but not for the `create` and `setData` which will be very interesting, even though we need to solve the problem of creating the parent nodes.
>
> I'll submit a different PR with the logic for the write/create
This is WIP. Adding create and setData is pretty easy based on this PR.
I plan to get a comparison result of containing batch read operations first, as we can see in previous zk perf result #12812, batch reading have the most performance improvements.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#issuecomment-982399675
> @Jason918 I've added a different PR based on the approach I was mentioning: #13043. PTAL
OK. This seems to be a duplicated work. Let's continue this work on your 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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #12975: [Issue 12812] Introduce ZKBatchedMetadataStore.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12975:
URL: https://github.com/apache/pulsar/pull/12975#discussion_r758690938
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKBatchedMetadataStore.java
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.pulsar.metadata.impl;
+
+
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pulsar.metadata.api.GetResult;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.impl.batch.BatchWorker;
+import org.apache.pulsar.metadata.impl.batch.MetadataOp;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+
+@Slf4j
+public class ZKBatchedMetadataStore extends ZKMetadataStore implements AsyncCallback.MultiCallback {
+
+ protected final boolean metadataStoreBatchingEnable = true;
+ protected final int metadataStoreBatchingMaxDelayMillis = 5;
+ protected final int metadataStoreBatchingMaxOperations = 100;
+ protected final int metadataStoreBatchingMaxSizeKb = 128;
Review comment:
These parameters should be left configurable, in particular the enable/disable, as we might want to disable in case we see bugs.
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKBatchedMetadataStore.java
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.pulsar.metadata.impl;
+
+
+import java.util.List;
+import java.util.Optional;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.pulsar.metadata.api.GetResult;
+import org.apache.pulsar.metadata.api.MetadataStoreConfig;
+import org.apache.pulsar.metadata.api.MetadataStoreException;
+import org.apache.pulsar.metadata.impl.batch.BatchWorker;
+import org.apache.pulsar.metadata.impl.batch.MetadataOp;
+import org.apache.zookeeper.AsyncCallback;
+import org.apache.zookeeper.KeeperException.Code;
+import org.apache.zookeeper.Op;
+import org.apache.zookeeper.OpResult;
+import org.apache.zookeeper.ZooDefs;
+
+@Slf4j
+public class ZKBatchedMetadataStore extends ZKMetadataStore implements AsyncCallback.MultiCallback {
Review comment:
Instead of extending the `ZKMetadataStore`, I believe we should have a common abstract class that implements the batching logic, which could be shared across multiple implementations.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org