You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/02/22 05:56:02 UTC
[GitHub] [bookkeeper] lordcheng10 opened a new pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
lordcheng10 opened a new pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072
### Motivation
Cleanup all the buckets that were in Deleted state, so that we can reduce unnecessary expansions.
### Changes
when removing，Cleanup all the buckets that were in Deleted state.
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1048090266
@eolivelli @merlimat 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#discussion_r815286262
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMap.java
##########
@@ -377,6 +377,16 @@ private V remove(long key, Object value, int keyHash) {
if (nextValueInArray == EmptyValue) {
values[bucket] = (V) EmptyValue;
--usedBuckets;
+
+ // Cleanup all the buckets that were in `DeletedValue` state,
Review comment:
Fixed
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 removed a comment on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1047451487
@eolivelli @merlimat 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1047451487
@eolivelli @merlimat 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 edited a comment on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1051833829
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1064718307
> Merged to master branch. Feel free to add the tests if you want as follow up work. Not strictly needed
OK, I will add unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1047949861
rerun 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1047760282
rerun 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1051833829
> Do we have tests about DeletedValue and the raw contents of the structures?
Is it worth to add some little test?
We can use PowerMock/Whitebox or pure java reflection to access the internals
@eolivelli
Didn't understand what you meant? Add a test to verify that the number of expansions has decreased?
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on a change in pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on a change in pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#discussion_r813002575
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMap.java
##########
@@ -377,6 +377,16 @@ private V remove(long key, Object value, int keyHash) {
if (nextValueInArray == EmptyValue) {
values[bucket] = (V) EmptyValue;
--usedBuckets;
+
+ // Cleanup all the buckets that were in `DeletedValue` state,
Review comment:
OK
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1053683592
No need to add a new flag.
You can build the test that inspects the internals of the structure.
You can test locally that your change works by rolling back the changes but keeping the test.
Without your changes the test should fail.
Adding a tunable is useless in this case as your fix is always a good thing and I guess nobody will disable 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1053684031
Merged to master branch.
Feel free to add the tests if you want as follow up work. Not strictly needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#discussion_r812589754
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/util/collections/ConcurrentLongHashMap.java
##########
@@ -377,6 +377,16 @@ private V remove(long key, Object value, int keyHash) {
if (nextValueInArray == EmptyValue) {
values[bucket] = (V) EmptyValue;
--usedBuckets;
+
+ // Cleanup all the buckets that were in `DeletedValue` state,
Review comment:
It looks like we can create a common private method to reduce code duplication in this class
Smaller methods are better in java
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1064743953
> Merged to master branch. Feel free to add the tests if you want as follow up work. Not strictly needed
Done: https://github.com/apache/bookkeeper/pull/3092
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1053443850
> > Do we have tests about DeletedValue and the raw contents of the structures?
> > Is it worth to add some little test?
> > We can use PowerMock/Whitebox or pure java reflection to access the internals
>
> @eolivelli Do you mean to add some tests to verify that the count of expansions is actually reduced?
Yes
--
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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] lordcheng10 commented on pull request #3072: reduce unnecessary expansions for ConcurrentLong map and set
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #3072:
URL: https://github.com/apache/bookkeeper/pull/3072#issuecomment-1053548491
> > > Do we have tests about DeletedValue and the raw contents of the structures?
> > > Is it worth to add some little test?
> > > We can use PowerMock/Whitebox or pure java reflection to access the internals
> >
> >
> > @eolivelli Do you mean to add some tests to verify that the count of expansions is actually reduced?
>
> Yes
@eolivelli
If I want to verify whether the number of expansions is reduced through unit testing, then I need to add a new configuration, such as: isCleanDeletedStatus=true, to control whether the new logic is enabled, so as to compare whether the number of expansions is reduced before and after it is enabled. 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: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org