-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add https to druid-influxdb-emitter extension #9938
Add https to druid-influxdb-emitter extension #9938
Conversation
result = 31 * result + getProtocol().hashCode(); | ||
result = 31 * result + (getTrustStorePath() != null ? getTrustStorePath().hashCode() : 0); | ||
result = 31 * result + (getTrustStoreType() != null ? getTrustStoreType().hashCode() : 0); | ||
result = 31 * result + (getTrustStorePassword() != null ? getTrustStorePassword().hashCode() : 0); |
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.
Looks like CI is complaining about a lack of test coverage. You can add an EqualsVerifier
test to fix that
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.
How does EqualsVerifier
work? The test I added is failing on
Non-nullity: equals throws NullPointerException on field port.
but if port is null then it gets set to a default value in the config's constructor.
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.
EqualsVerifier doesn't know that because that field is not marked as NotNull
. There's more information about that exception here - https://jqno.nl/equalsverifier/errormessages/non-nullity-equals-hashcode-tostring-throws-nullpointerexception/
You could also tell the test that the field is not null with .withNonnullFields("port")
in the EqualsVerifier test
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.
Apologies for the delayed review, any chance you could update conflicts so we can get this merged?
@@ -130,6 +159,10 @@ public int hashCode() | |||
{ | |||
int result = getHostname().hashCode(); | |||
result = 31 * result + getPort(); | |||
result = 31 * result + getProtocol().hashCode(); |
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.
Is there any reason not to use
return Objects.hash(
return Objects.hash(
hostname,
port,
...
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions. |
This pull request/issue is no longer marked as stale. |
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.
@awelsh93, apologize for delayed review. I left comments on exception handling.
throw new IllegalStateException(msg); | ||
} | ||
finally { | ||
if (in != null) { |
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.
It would be better to use try-with-resources
. You will not have to handle the IOException thrown in close()
by yourself.
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 your review - I think I've addressed these comments. Can you take another look please?
in.close(); | ||
} | ||
catch (IOException ex) { | ||
log.error("Unable to load TrustStore"); |
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.
I think this should be thrown.
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.
+1 after CI. @awelsh93 thank you for your patience!
* Add https to druid-influxdb-emitter extension * address CI failures * increase test coverage * tests for being unable to load trustStore * fix EqualsVerifier test * fix intellij inspection error * use try-with-resources when loading trustStore
Fixes #9895.
This PR has: