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.

Sign up for GitHub

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

Add read timeout for how long we query beans #168

Merged
merged 5 commits into from
Mar 7, 2018
Merged

Add read timeout for how long we query beans #168

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23066ff
Add read timeout for how long we query beans
nmuesch Mar 7, 2018
31d8e92
Changes default to 15 and tries to use refresh bean as refresh period
nmuesch Mar 7, 2018
c477c87
Use the new value
nmuesch Mar 7, 2018
647736b
Update timeout to be that of the refresh_bean default
nmuesch Mar 7, 2018
25b7061
Uses its own yaml parameter, defaults back 15 seconds
nmuesch Mar 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/org/datadog/jmxfetch/RemoteConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class RemoteConnection extends Connection {
private String jmx_url;
private static final String TRUST_STORE_PATH_KEY = "trust_store_path";
private static final String TRUST_STORE_PASSWORD_KEY = "trust_store_password";
private static final String DEFAULT_RMI_RESPONSE_TIMEOUT = "5000";
Copy link
Member

Choose a reason for hiding this comment

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

based on the docs this is expressed in milliseconds, are you sure 5 seconds is enough in usual cases? (I know some JVMs expose a lot of beans, and on these environments I could imagine that the call to get all the beans takes more than 5 seconds).

Copy link
Contributor Author

@nmuesch nmuesch Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I bumped this up to 15 by default and am using the user defined "refresh_beans" parameter to address this and make it configurable. Though it may make more sense to actually have this set to the min_collection_interval. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bumped this again to actually be the default of the refresh_bean timeout as I think that makes the most sense here.

private final static Logger LOGGER = Logger.getLogger(Connection.class.getName());

public RemoteConnection(LinkedHashMap<String, Object> connectionParams)
Expand Down Expand Up @@ -53,6 +54,10 @@ public RemoteConnection(LinkedHashMap<String, Object> connectionParams)
}

}

//Set an RMI timeout so we don't get stuck waiting for a bean to report a value
System.setProperty("sun.rmi.transport.tcp.responseTimeout", DEFAULT_RMI_RESPONSE_TIMEOUT);
Copy link
Member

Choose a reason for hiding this comment

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

what happens when we reach the timeout? Do we catch and handle the exception that's raised correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error should be shown during the refresh bean list period where we scope out all available beans. If we hit the timeout there, an exception will be thrown (with a read timeout) and we should wait until the next collection period to try again.


createConnection();

}
Expand Down