Skip to content

Reduce Guava usage - #2026

Closed
pivovarit wants to merge 4 commits into
testcontainers:masterfrom
pivovarit:reduce-guava
Closed

Reduce Guava usage#2026
pivovarit wants to merge 4 commits into
testcontainers:masterfrom
pivovarit:reduce-guava

Conversation

@pivovarit

@pivovarit pivovarit commented Nov 2, 2019

Copy link
Copy Markdown
Contributor

Not sure if removing Guava dependency is something on your roadmap, but there are multiple spots where its usage doesn't introduce much value, for example:

  1. Charsets.UTF_8 is already provided by Java as StandardCharsets.UTF_8
  2. Sets.newConcurrentHashSet() doesn't provide any extra value over a simple synchronized collection (in those particular spots)
  3. Maps.newHashMap() is just a wrapped HashMap's constructor

HttpUrl baseUrl;

ImmutableList<String> path;
List<String> path;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The class is package-private so no backward compatibility issues

private Set<String> registeredNetworks = Sets.newConcurrentHashSet();
private Set<String> registeredImages = Sets.newConcurrentHashSet();
private final Map<String, String> registeredContainers = new ConcurrentHashMap<>();
private final Set<String> registeredNetworks = synchronizedSet(new HashSet<>());

@pivovarit pivovarit Nov 2, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sets.newConcurrentHashSet wraps ConcurrentHashMap and exposes it as Set - we don't really benefit from its fine-grained locking here so I used a classic synchronized HashSet

@pivovarit
pivovarit marked this pull request as ready for review November 2, 2019 15:53
@bsideup

bsideup commented Nov 6, 2019

Copy link
Copy Markdown
Member

@pivovarit

Thanks for submitting this PR! The Guava dependency comes from docker-java (and we shade it later) so we don't plan to remove it unless docker-java drops it (Soon, btw)

Since the PR's title says "reduce", I wonder if there are more uses left?
I believe we should either remove every use of Guava altogether, probably when we update to docker-java 3.2.0, not just some.

@pivovarit

Copy link
Copy Markdown
Contributor Author

@bsideup sure, I can work towards removing all of it - wanted to probe your thoughts first!

@pivovarit pivovarit closed this Nov 8, 2019
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