Skip to content

Conversation

@j3grewal
Copy link

DNS lookup is done on every reconnect attempt. Caching the machine address for the lifetime of the application is problematic in a dynamic environment like docker containers. If the machine address changes, applications need to be restarted to pick up the correct address and log correctly.

…dress for the lifetime of the application is problematic in a dynamic enviroment like docker containers. If the machine address changes, applications need to be restarted to pick up the correct address and log correctly.

private String name;

private String host;
Copy link
Member

Choose a reason for hiding this comment

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

Use final keyword


private String host;

private int port;
Copy link
Member

Choose a reason for hiding this comment

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

Use final keyword

pendings = ByteBuffer.allocate(bufferCapacity);
this.host = host;
this.port = port;
server = new InetSocketAddress(host, port);
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed


private String name;

private String host;
Copy link
Member

Choose a reason for hiding this comment

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

server can be removed since the instance variable in the following line can be replaced with socket.getInetAddress().

LOG.error("Cannot send logs to " + server);

@komamitsu
Copy link
Member

@j3grewal Thanks!

I left some comments. Could you address them?
If it's difficult to fix them, let me know that and I'll do that later.

BTW, as you may know, InetSocketAddress(String hostname, int port) internally uses InetAddress.getByName(String host) and it caches a resolved address. So if you want to shorten the TTL of the caching, set networkaddress.cache.ttl to a shorter time.

@j3grewal
Copy link
Author

Thanks @komamitsu

I have made the changes as per your comments.

Yes, we had tried setting the networkaddress.cache.ttl but it didn't solve the problem because of the caching in this particular class.

@komamitsu komamitsu merged commit a8021dd into fluent:master Jul 22, 2017
@j3grewal
Copy link
Author

Thanks for merging @komamitsu

What are your plans for the next release?

@komamitsu
Copy link
Member

@j3grewal I'll release a new version later this week.

BTW, I noticed https://github.com/komamitsu/fluency (Yet another fluent logger Java) already supports this feature. Also, it has some other features. I'd be happy if you try it when you get a chance :)

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