Add https to druid-influxdb-emitter extension#9938
Add https to druid-influxdb-emitter extension#9938clintropolis merged 8 commits intoapache:masterfrom
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.
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.
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.
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
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
Apologies for the delayed review, any chance you could update conflicts so we can get this merged?
| { | ||
| int result = getHostname().hashCode(); | ||
| result = 31 * result + getPort(); | ||
| result = 31 * result + getProtocol().hashCode(); |
There was a problem hiding this comment.
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. |
| throw new IllegalStateException(msg); | ||
| } | ||
| finally { | ||
| if (in != null) { |
There was a problem hiding this comment.
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.
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.
I think this should be thrown.
* 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
4 participants
Fixes #9895.
This PR has: