Skip to content
This repository was archived by the owner on Mar 27, 2021. It is now read-only.

Conversation

@lmuhlha
Copy link
Member

@lmuhlha lmuhlha commented Jul 20, 2020

No description provided.

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #679 into master will decrease coverage by 0.03%.
The diff coverage is 25.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #679      +/-   ##
============================================
- Coverage     53.91%   53.87%   -0.04%     
+ Complexity     2964     2961       -3     
============================================
  Files           726      726              
  Lines         19423    19426       +3     
  Branches       1277     1279       +2     
============================================
- Hits          10472    10466       -6     
- Misses         8508     8514       +6     
- Partials        443      446       +3     
Impacted Files Coverage Δ Complexity Δ
...ic/elasticsearch/AbstractElasticsearchBackend.java 64.28% <0.00%> (-4.95%) 3.00 <0.00> (ø)
...heroic/suggest/elasticsearch/SuggestBackendKV.java 80.16% <50.00%> (-0.98%) 61.00 <0.00> (-1.00)
...com/spotify/heroic/aggregation/simple/MaxBucket.kt 44.44% <0.00%> (-11.12%) 4.00% <0.00%> (-1.00%)
...com/spotify/heroic/aggregation/simple/MinBucket.kt 44.44% <0.00%> (-11.12%) 4.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2e6baa...3f6077c. Read the comment docs.

@lmuhlha lmuhlha force-pushed the droppedbydup branch 2 times, most recently from 4099746 to a86c7ae Compare July 20, 2020 17:16
@malish8632
Copy link
Contributor

@lmuhlha just to make sure I didn't miss it. The biggest changes I see you removed try() for tracer.withSpan and rapped connection.doto to private doto. Is there anything else big I've missed?

@lmuhlha
Copy link
Member Author

lmuhlha commented Jul 20, 2020

@malish8632 most of the changes are cosmetic when i was comparing the classes. The important changes that provide the fix are
https://github.com/spotify/heroic/pull/679/files#diff-96ff9d0380bf9df05d420452e2a824eaR57
and
https://github.com/spotify/heroic/pull/679/files#diff-1290b46b7a9f097ecbf07d898c73e788R681

The problem was that the version conflict format changed from the transport client to the rest client, and on top of that it appears to be nested inside another error. So it was being caught as a failure rather than dropped by duplicate.

I can revert the cosmetic changes if need be, I just left them because I liked the more similar formatting.

Copy link

@smstone smstone left a comment

Choose a reason for hiding this comment

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

Is it possible to put cosmetic changes in a separate commit?

) {
return throwable -> {
if (ExceptionUtils.getRootCause(throwable) instanceof VersionConflictEngineException) {
if (throwable.getMessage().contains("version conflict")) {
Copy link

Choose a reason for hiding this comment

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

Since we know what exceptions we want to deal with, why not just have a list of exceptions we act on, rather than string matching? I just wonder if the text will change at some point, which would cause this to break.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's what I was thinking as well. I think I tried going into the nested exception but I couldn't the other day.
Let me run it locally again and grab the exact errors and try again.

@lmuhlha lmuhlha force-pushed the droppedbydup branch 3 times, most recently from 1aae450 to a74602a Compare July 20, 2020 22:00
@lmuhlha lmuhlha force-pushed the droppedbydup branch 4 times, most recently from ae58dc3 to 9473eba Compare July 21, 2020 19:43
@lmuhlha lmuhlha force-pushed the droppedbydup branch 3 times, most recently from 5e64236 to 19def0f Compare July 21, 2020 20:29
@lmuhlha lmuhlha merged commit 6353fef into master Jul 21, 2020
@delete-merged-branch delete-merged-branch bot deleted the droppedbydup branch July 21, 2020 20:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants