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/19 14:23:41 UTC
[GitHub] [pulsar] Jason918 opened a new pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Jason918 opened a new pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- 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 #12894
### Motivation
See #12894
### Modifications
Only refresh metadata if path is already in cache after write.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is already covered by existing tests, such as *(please describe tests)*.
### 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`
bug fix.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753344157
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -331,6 +331,7 @@ public void insertionOutsideCacheWithGenericType(String provider, Supplier<Strin
});
String key1 = newKey();
+ objCache.get(key1).join();
Review comment:
This is forcing the key to be in the cache, but the test was expressly trying to check the condition in which it was not.
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java
##########
@@ -155,4 +155,12 @@
* @param path the path of the object in the metadata store
*/
void refresh(String path);
+
+
+ /**
+ * Check if the value of this path is already in cache.
+ * @param path the path of the object in the metadata store
+ * @return true if the path exists.
+ */
+ boolean existsInCache(String path);
Review comment:
I wouldn't add a new method in the API that we only are going to be using internally.
Two solutions:
1. There is already `MetadataCache.getIfCached()`
2. `MetadaCache.refresh()` implementation should take care of it internally.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-979676884
@eolivelli PTAL. 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] eolivelli commented on a change in pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755826887
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -324,13 +324,14 @@ public void insertionOutsideCache(String provider, Supplier<String> urlSupplier)
}
@Test(dataProvider = "impl")
- public void insertionOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
+ public void updateOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
Review comment:
why are you changing an existing test ?
can we add a new test ?
how are you verifying your change ?
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
##########
@@ -261,8 +261,11 @@ public void invalidate(String path) {
@Override
public void refresh(String path) {
- objCache.synchronous().invalidate(path);
- objCache.synchronous().refresh(path);
+ // Refresh object of path if only it is cached before.
+ if (objCache.getIfPresent(path) != null) {
Review comment:
we should move this block into a dedicated method of objCache
`objCache.synchronous().invalidateAndRefreshIfPresent(path)`
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755914748
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
##########
@@ -261,8 +261,11 @@ public void invalidate(String path) {
@Override
public void refresh(String path) {
- objCache.synchronous().invalidate(path);
- objCache.synchronous().refresh(path);
+ // Refresh object of path if only it is cached before.
+ if (objCache.getIfPresent(path) != null) {
Review comment:
> I'm just wondering if there's a possibility for races.
No, it's ok to miss a refresh or duplicate a refresh. Worst case is you have to do an extra loading when calling get.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619915
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java
##########
@@ -155,4 +155,12 @@
* @param path the path of the object in the metadata store
*/
void refresh(String path);
+
+
+ /**
+ * Check if the value of this path is already in cache.
+ * @param path the path of the object in the metadata store
+ * @return true if the path exists.
+ */
+ boolean existsInCache(String path);
Review comment:
The only reason I am not using `MetadataCache.getIfCached()` is it would block and wait for current load future is completed.
I am taking solution 2. It would not be "refresh" if we have not already loaded the path object, right?
Although, it may change some old behaviors.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619596
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -331,6 +331,7 @@ public void insertionOutsideCacheWithGenericType(String provider, Supplier<Strin
});
String key1 = newKey();
+ objCache.get(key1).join();
Review comment:
Change the test to `updateOutsideCacheWithGenericType`.
For now, I think if we can implement `insertionOutsideCacheWithGenericType` by make a path pattern registration or add a method like `accept(path)` for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755917562
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
##########
@@ -261,8 +261,11 @@ public void invalidate(String path) {
@Override
public void refresh(String path) {
- objCache.synchronous().invalidate(path);
- objCache.synchronous().refresh(path);
+ // Refresh object of path if only it is cached before.
+ if (objCache.getIfPresent(path) != null) {
Review comment:
> we should move this block into a dedicated method of objCache
>
> `objCache.synchronous().invalidateAndRefreshIfPresent(path)`
It's a bit of trouble to do this, because objCache is an instance of class `com.github.benmanes.caffeine.cache.AsyncCacheLoader` , which is in a dependent lib.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-982317761
@merlimat PTAL. Thanks.
And to implement the origin "insertionOutsideCacheWithGenericType", I think we can add a path pattern registration or add a method like accept(path) for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store no matter how it's updated.
What do you think?
--
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] lhotari commented on a change in pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755791588
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/cache/impl/MetadataCacheImpl.java
##########
@@ -261,8 +261,11 @@ public void invalidate(String path) {
@Override
public void refresh(String path) {
- objCache.synchronous().invalidate(path);
- objCache.synchronous().refresh(path);
+ // Refresh object of path if only it is cached before.
+ if (objCache.getIfPresent(path) != null) {
Review comment:
I'm just wondering if there's a possibility for races.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619915
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java
##########
@@ -155,4 +155,12 @@
* @param path the path of the object in the metadata store
*/
void refresh(String path);
+
+
+ /**
+ * Check if the value of this path is already in cache.
+ * @param path the path of the object in the metadata store
+ * @return true if the path exists.
+ */
+ boolean existsInCache(String path);
Review comment:
The only reason I am not using `MetadataCache.getIfCached()` is it would block and wait for current load future is completed.
I am taking solution 2. It would be "refresh" if we have not already loaded the path object. right?
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619915
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java
##########
@@ -155,4 +155,12 @@
* @param path the path of the object in the metadata store
*/
void refresh(String path);
+
+
+ /**
+ * Check if the value of this path is already in cache.
+ * @param path the path of the object in the metadata store
+ * @return true if the path exists.
+ */
+ boolean existsInCache(String path);
Review comment:
The only reason I am not using `MetadataCache.getIfCached()` is it would block and wait for current load future is completed.
I am taking solution 2. It would not be "refresh" if we have not already loaded the path object, right?
Although, it may change some old behaviors.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619915
##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataCache.java
##########
@@ -155,4 +155,12 @@
* @param path the path of the object in the metadata store
*/
void refresh(String path);
+
+
+ /**
+ * Check if the value of this path is already in cache.
+ * @param path the path of the object in the metadata store
+ * @return true if the path exists.
+ */
+ boolean existsInCache(String path);
Review comment:
The only reason I am not using `MetadataCache.getIfCached()` is it would block and wait for current load future is completed.
I am taking solution 2. It would be "refresh" if we have not already loaded the path object. right?
Although, it may change some old behaviors.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r753619596
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -331,6 +331,7 @@ public void insertionOutsideCacheWithGenericType(String provider, Supplier<Strin
});
String key1 = newKey();
+ objCache.get(key1).join();
Review comment:
Changed the test to `updateOutsideCacheWithGenericType` for now.
I think we can implement `insertionOutsideCacheWithGenericType` by make a path pattern registration or add a method like `accept(path)` for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755924835
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -324,13 +324,14 @@ public void insertionOutsideCache(String provider, Supplier<String> urlSupplier)
}
@Test(dataProvider = "impl")
- public void insertionOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
+ public void updateOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
Review comment:
> how are you verifying your change ?
This test is modified to verify my change.
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-976096862
@merlimat please help take a look?
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#discussion_r755924005
##########
File path: pulsar-metadata/src/test/java/org/apache/pulsar/metadata/MetadataCacheTest.java
##########
@@ -324,13 +324,14 @@ public void insertionOutsideCache(String provider, Supplier<String> urlSupplier)
}
@Test(dataProvider = "impl")
- public void insertionOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
+ public void updateOutsideCacheWithGenericType(String provider, Supplier<String> urlSupplier) throws Exception {
Review comment:
> Changed the test to `updateOutsideCacheWithGenericType` for now.
>
> I think we can implement `insertionOutsideCacheWithGenericType` by make a path pattern registration or add a method like `accept(path)` for each MetadataStore instance, which implies the metadata store could deserialization this value, and the path indeed should be cached in this metadata store.
Please have a look at the conversation above with merlimat.
I am actually changing the feature for now, for a quick fix of the warning logs.
More complex solution is need to achieve the goal of **insertionOutsideCacheWithGenericType**.
--
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] Shoothzj commented on pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-978643223
comment #12788 for link
--
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] lhotari merged pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-974120128
@merlimat PTAL.
We have dozens of `MetadataCacheImpl` instances in `AbstractMetadataStore#metadataCaches`.
Each of these MetadataCacheImpl have different type of object to cache, so we can't refresh them all with the same path.
--
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] eolivelli commented on pull request #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-983481014
+1
--
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 #12896: [Issue 12894] Only refresh metadata if path is already in cache after write.
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #12896:
URL: https://github.com/apache/pulsar/pull/12896#issuecomment-976213983
@315157973 Here is the fix of #12909, 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