Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove and forbid use of com.google.common.collect.EvictingQueue #13903

Merged
merged 2 commits into from Oct 8, 2015
Merged

Remove and forbid use of com.google.common.collect.EvictingQueue #13903

merged 2 commits into from Oct 8, 2015

Conversation

jasontedor
Copy link
Member

This commit removes and now forbids all uses of
com.google.common.collect.EvictingQueue across the codebase. This is
one of the few remaining steps in the eventual removal of Guava as a
dependency.

Relates #13224

@jpountz
Copy link
Contributor

jpountz commented Oct 2, 2015

Wouldn't it be better to change our pipeline aggs to use a regular queue and evict explicitly instead of adding this new queue impl? This would avoid this new abstraction.

@jasontedor
Copy link
Member Author

@jpountz I took a look at whether or not manually managing evictions in the pipeline aggregations code would simplify things or not, and frankly I think it doesn't and I think it would lead to code that is harder to maintain. In this case, I think that this (relatively simple) abstraction simplifies the pipeline aggregations code (so that there isn't the noise of manually managing the evictions there) and makes it more maintainable (future maintainers have to take care to properly manage any changes to the add logic). I would be interested in seeing if you have a simple and maintainable way that I'm not seeing?

@jpountz
Copy link
Contributor

jpountz commented Oct 7, 2015

I haven't really tried or even looked, I was just thinking out loud. If it looks easier with this abstraction, I'm good with the change. Can you just add some minimal javadocs to EvictingQueue before pushing?

This commit removes and now forbids all uses of
com.google.common.collect.EvictingQueue across the codebase. This is
one of the few remaining steps in the eventual removal of Guava as a
dependency.

Relates #13224
@jasontedor
Copy link
Member Author

@jpountz Added Javadocs in e61e746.

jasontedor added a commit that referenced this pull request Oct 8, 2015
Remove and forbid use of com.google.common.collect.EvictingQueue
@jasontedor jasontedor merged commit 23cd64b into elastic:master Oct 8, 2015
@jasontedor jasontedor deleted the evicting-queue-be-gone branch October 8, 2015 01:05
@jasontedor
Copy link
Member Author

Thank you for reviewing @jpountz.

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

Successfully merging this pull request may close these issues.

None yet

2 participants