Non-final method invocation in constructor #38149
Conversation
…cts/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java#x4e06dc48702dac54:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#xaeb362a26c0fdaf1:1) * [Non-final method invocation in constructor](https://lgtm.com/projects/g/elastic/elasticsearch/snapshot/dist-1916470085-1548143539391/files/server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java?sort=name&dir=ASC&mode=heatmap#x5c00888239d02f3a:1)
colings86
added
the
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
Thanks @krisds for this PR. I can see the problem in the constructor and your solution looks like it would solve that. However, I would like to propose to do this in a different way: move the offending parts in BlobStoreRepository out of the constructor and into the doStart method instead. This should ensure correct initialization in a simpler way.
Did you look into writing a unit test provoking an issue for this? I suspect, we would not error on bad chunksize settings, that should be relatively easy to provoke.
|
Thanks for the tip, @henningandersen. I'll try a rewrite as suggested. Any suggestions on where a unit test for this should go ? Any existing unit test I could try to base this on ? |
|
I think this new test belongs in |
|
I hope this is about what you expected. I'm not familiar enough with the project to make sure the expected test result is correct. It rejects bad values for chunksize, which make some sense to me, but please doublecheck. |
Added test case ensuring that compress and chunkSize are not called in the BlobStoreRepository constructor, since this is unsafe. Plus fixed testBadChunks to only use illegal chunkSizes for verification.
|
Thanks for your update, looks good. Added one more test and a small correction, have pushed this back to your branch for inclusion into this PR. |
|
test this please |
Fixed test case to not rely on Test annotation.
|
test this please |
|
Looks good to me. Test passes on my machine. |
|
@elasticmachine test this please |
|
I have had a second set of eyes on this and our suggestion is to fix the compress problem by instead passing it into the constructor and moving the @krisds, do you want to carry out this work? If this is larger than what you intended with your contribution, we can also take over this work. |
|
@henningandersen, I don't mind if you want to take over. It will be a few days before I can get back to this myself anyway. As long as we get a nice fix, I'm happy. |
As requested, I will split #38033 into separate PRs. Here is the first part.
This one resolves: