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 2022/06/10 12:04:07 UTC
[GitHub] [pulsar] Jason918 opened a new pull request, #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Jason918 opened a new pull request, #16005:
URL: https://github.com/apache/pulsar/pull/16005
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
Fixes #15896
### Motivation
See #15896
When the same key is modified concurrently, there will be LockTimeout exception in `RocksdbMetadataStore`.
The root cause is we haven't get the lock of the key when read old data in `store.getForUpdate(...)`, so the exception throws when two transaction modifies the same key.
### Modifications
Set `exclusive` parameter of `getForUpdate` to `true` in `RocksdbMetadataStore#storePut`.
The same applies to `RocksdbMetadataStore#storeDelete`.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- org.apache.pulsar.metadata.MetadataStoreTest#testConcurrentPut
- org.apache.pulsar.metadata.MetadataStoreTest#testConcurrentDelete
### 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`
--
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 diff in pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#discussion_r894972807
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java:
##########
@@ -447,4 +448,33 @@ public void run() {
assertEquals(successWrites.get(), maxValue);
assertEquals(store.get(path).get().get().getValue()[0], maxValue);
}
+
+ @Test(dataProvider = "impl")
+ public void testConcurrentPut(String provider, Supplier<String> urlSupplier) throws Exception {
+ MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
Review Comment:
Yep, added.
--
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] codelipenghui commented on a diff in pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#discussion_r894488726
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java:
##########
@@ -447,4 +448,33 @@ public void run() {
assertEquals(successWrites.get(), maxValue);
assertEquals(store.get(path).get().get().getValue()[0], maxValue);
}
+
+ @Test(dataProvider = "impl")
+ public void testConcurrentPut(String provider, Supplier<String> urlSupplier) throws Exception {
+ MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
Review Comment:
Need to close the store after test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #16005:
URL: https://github.com/apache/pulsar/pull/16005
--
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 #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#issuecomment-1155890625
@merlimat 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] github-actions[bot] commented on pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#issuecomment-1152302296
@Jason918:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#issuecomment-1153058994
/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 #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#issuecomment-1154672738
@codelipenghui @wuxuanqicn 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] wuxuanqicn commented on a diff in pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
wuxuanqicn commented on code in PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#discussion_r894598868
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java:
##########
@@ -447,4 +448,33 @@ public void run() {
assertEquals(successWrites.get(), maxValue);
assertEquals(store.get(path).get().get().getValue()[0], maxValue);
}
+
+ @Test(dataProvider = "impl")
+ public void testConcurrentPut(String provider, Supplier<String> urlSupplier) throws Exception {
+ MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
+
+ String k = newKey();
+ CompletableFuture<Void> f1 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
+ CompletableFuture<Void> f2 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
Review Comment:
more operations are needed, if we are testing concurrent operation,
https://github.com/apache/pulsar/blob/master/pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/GrowablePriorityLongPairQueueTest.java#L164-L187
--
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 #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#issuecomment-1152852900
/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 diff in pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#discussion_r894973097
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java:
##########
@@ -447,4 +448,33 @@ public void run() {
assertEquals(successWrites.get(), maxValue);
assertEquals(store.get(path).get().get().getValue()[0], maxValue);
}
+
+ @Test(dataProvider = "impl")
+ public void testConcurrentPut(String provider, Supplier<String> urlSupplier) throws Exception {
+ MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
+
+ String k = newKey();
+ CompletableFuture<Void> f1 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
+ CompletableFuture<Void> f2 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
Review Comment:
Do you mean increase concurrency?
It's not necessary in this case, two threads can reproduce the issue easily.
--
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] wuxuanqicn commented on a diff in pull request #16005: [Issue 15896] Fix LockTimeout when storePut on the same key concurrently in RocksdbMetadataStore
Posted by GitBox <gi...@apache.org>.
wuxuanqicn commented on code in PR #16005:
URL: https://github.com/apache/pulsar/pull/16005#discussion_r894598868
##########
pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataStoreTest.java:
##########
@@ -447,4 +448,33 @@ public void run() {
assertEquals(successWrites.get(), maxValue);
assertEquals(store.get(path).get().get().getValue()[0], maxValue);
}
+
+ @Test(dataProvider = "impl")
+ public void testConcurrentPut(String provider, Supplier<String> urlSupplier) throws Exception {
+ MetadataStore store = MetadataStoreFactory.create(urlSupplier.get(), MetadataStoreConfig.builder().build());
+
+ String k = newKey();
+ CompletableFuture<Void> f1 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
+ CompletableFuture<Void> f2 =
+ CompletableFuture.runAsync(() -> store.put(k, new byte[0], Optional.of(-1L)).join());
Review Comment:
more operation is needed, if we are testing concurrent operation,
https://github.com/apache/pulsar/blob/master/pulsar-common/src/test/java/org/apache/pulsar/common/util/collections/GrowablePriorityLongPairQueueTest.java#L164-L187
--
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