Skip to content

updating containsKey method in PassiveExpiringMap#53

Open
thePatmanMI wants to merge 2 commits into
apache:masterfrom
thePatmanMI:patch-1
Open

updating containsKey method in PassiveExpiringMap#53
thePatmanMI wants to merge 2 commits into
apache:masterfrom
thePatmanMI:patch-1

Conversation

@thePatmanMI

Copy link
Copy Markdown

correcting the containsKey method to remove all expired entries to match the class and method javadoc statements

correcting the containsKey method to remove all expired entries to match the class and method javadoc statements

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a unit test that fails without this patch. Thank you!

@thePatmanMI

Copy link
Copy Markdown
Author

are there additional problems with this change, or can it be merged in?

@kinow kinow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks OK now I think, with a test. Before being merged, it would require a JIRA ticket as well, to be included in changes.xml.

I had a look at the code in PassiveExpiringMap, and the same removeIfExpired(key, now()) is used in other three methods. The javadocs do not clarify if the key argument is supposed to be used and clear only the expired values with that key, or not.

So for me the options here are to leave as-is and clarify in the Javadoc documentation what is cleared. Otherwise, to modify all methods to match the current javadocs and to expireAll instead.

@garydgregory

Copy link
Copy Markdown
Member

I am waiting for the comments from @kinow to be resolved, by @thePatmanMI or another.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants