-
Notifications
You must be signed in to change notification settings - Fork 23
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
Enable SSL, make reporter a daemon thread #118
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Overall looks good. I've left some comments. I would generally think it's best to separate Daemon Thread to a separate PR and issue
@@ -67,7 +67,7 @@ public void testOnServerRestart() throws InterruptedException { | |||
int connectTimeout = 1000; | |||
int socketTimeout = 1000; | |||
GraphiteClient client = new GraphiteClient("bla-host.com", "bla-service", "localhost", | |||
port, connectTimeout, socketTimeout, 20000, | |||
port, false, connectTimeout, socketTimeout, 20000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add an integration test to see that the functionality works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, setting up an ssl-enabled graphite is fairly complicated, and doing it in an integration test more so, and I don't have the time right now to get it working. I understand if you don't want to merge without a test though. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elahrvivaz Even if you use testcontainers? If you already did once for manual testing, it's only a matter of writing the configuration file for Graphite no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, well I didn't set up the instance I've been testing on, and I'm not familiar with testcontainers :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elahrvivaz Testcontainers is fairly easy to work with. You have a great example at GraphiteITest. Take a few minutes to review it - I'll happy to assist with any questions you may have.
@elahrvivaz Would you like to continue on this? |
I'll close it for now, if someone else wants to pick it up feel free! |
Closes #116