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 guava #482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

remove guava #482

wants to merge 1 commit into from

Conversation

joshgord
Copy link
Contributor

No description provided.

Copy link
Contributor

@brharrington brharrington left a comment

Choose a reason for hiding this comment

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

I'm worried this would be a breaking change for many internal libraries and apps at Netflix. Would need to understand the blast radius better. Will follow up next week.

import com.netflix.servo.tag.TaggingContext;

import java.util.function.Function;
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue with this is that Function is part of the public API for this class. See previous discussions in #288 and #290. We would need to make sure it wouldn't break anything internally. Since servo is deprecated, we would probably focus more on moving users off instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, I can look at fixing the breakages if they occur internally. I also took a quick look at who is still using servo and it's a bit intertwined into internal platform libraries which are also deprecated. I can also recommend full removal of servo but figured this would get some benefit in the interim.

@@ -15,8 +15,7 @@
*/
package com.netflix.servo.monitor;

import com.google.common.base.Function;
import com.google.common.cache.Cache;
import com.github.benmanes.caffeine.cache.Cache;
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no usage of Monitors.newCacheMonitor internally at Netflix that would break, then I would prefer to just remove the method rather than move it over to Caffeine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me, will remove vs update to Caffeine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like DynamicGauge uses it 🤔

/**
* Intern strings used for tag keys or values.
*/
public static String intern(String v) {
return STR_CACHE.intern(v);
return v.intern();
Copy link
Contributor

Choose a reason for hiding this comment

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

This would have very different performance implications and could cause issues. The default string table in the JDK doesn't grow and can be quite small unless customized by the user and having too many interned strings can really degrade performance.

https://shipilev.net/jvm/anatomy-quarks/10-string-intern/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I was hoping we could take the performance hit for legacy consumers or if they notice the problem we can get them to update to spectator 😆

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.

2 participants