elasticsearch: use backpressure to resolve streaming request flakes #3236

minwoox · 2020-10-15T03:10:20Z

The HttpResponse was sent before the request are fully sent.
So the request was aborted after getting the response. This changes
to send the response after the request is fully received.

Fixes #3197

minwoox · 2020-10-15T04:31:33Z

CI failed with the same cause in another test. 😅 Let me fix that too, after lunch. 😄

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchStorage.java

minwoox · 2020-10-15T14:14:49Z

Finally, it succeeded. 😅
Let me rebuild a couple of times though.

codefromthecrypt

Thanks I like the green color a lot!

Can you help with comments to ensure I understand?

Also, is it temporary that we have to check the isEndOfStream flag? EG, could armeria do that internally? If so, possibly mark an issue in a comment so we don't forget?

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java

...n-server/src/test/java/zipkin2/server/internal/elasticsearch/ITElasticsearchHealthCheck.java

minwoox · 2020-10-16T01:31:33Z

@adriancole Do you have a chance to rebuild this a couple of times? 😅

codefromthecrypt · 2020-10-16T01:33:16Z

I clicked rebuild.. you can also rebase into one commit off latest master and force push your branch. pretty confident so far

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java

benchmarks/pom.xml

codefromthecrypt · 2020-10-16T03:55:16Z

OK will merge on last green. the comment editing should show this is all working :D Massive thanks @minwoox!

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java

codefromthecrypt · 2020-10-16T05:50:10Z

if you don't mind, rebase and squash onto latest master. not a big deal if can't

minwoox · 2020-10-16T06:24:13Z

if you don't mind, rebase and squash onto latest master. not a big deal if can't

After I merged the master, I got this error while building
Some files do not have the expected license header
How can I check which files do not have the expected license?

codefromthecrypt · 2020-10-16T06:46:40Z

in .github/CONTRIBUTING.md there's this ./mvnw com.mycila:license-maven-plugin:format

I corrected the problematic license in master, but you can run it again after rebase anyway.

minwoox · 2020-10-16T06:49:35Z

Let me merge master again, then. 😄

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java

minwoox · 2020-10-22T02:44:43Z

Let me fix this #3239 in different PR.
Somehow it's not working. 😓

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java

codefromthecrypt

Exciting! If you have some time, lets get all 1.2 related in the same bucket?

wanna remove some of the log hushing in zipkin-server-shared.yml now?

(not sure which are needed anymore)

and maybe the TRACE workaround in ZipkinHttpConfiguration

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java

minwoox · 2020-10-22T03:41:06Z

and maybe the TRACE workaround in ZipkinHttpConfiguration

I left a comment on this. 😄 #3236 (comment)
Let me solve it in different PR.

codefromthecrypt · 2020-10-22T08:43:51Z

[INFO] -------------------------------------------------------

[INFO] Running zipkin2.elasticsearch.integration.ITElasticsearchStorageV6

2020-10-22 08:05:47:143 [main] WARN org.testcontainers.utility.RegistryAuthLocator - Failure when attempting to lookup auth config (dockerImageName: testcontainers/ryuk:0.3.0, configFile: /home/travis/.docker/config.json. Falling back to docker-java default behaviour. Exception message: /home/travis/.docker/config.json (No such file or directory)

2020-10-22 08:06:14:605 [main] WARN zipkin2.elasticsearch.integration.ITElasticsearchStorage - The ElasticSearch instance used during IT's is using deprecated features or configuration. This is likely nothing to be really worried about (for example 'xpack.monitoring.enabled' setting), but nevertheless it should be looked at to see if our docker image used during integration tests needs updating for the next version of ElasticSearch. See https://www.elastic.co/guide/en/elasticsearch/reference/current/migration-api-deprecation.htmlfor more information. This is the deprecation warning we received:

{"error":"Incorrect HTTP method for uri [/_migration/deprecations] and method [GET], allowed: [POST]","status":405}

[INFO] Running zipkin2.elasticsearch.integration.ITElasticsearchStorage$ITDependenciesHeavy

...-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/client/HttpCallTest.java

darkmastermindz · 2023-08-17T22:03:47Z

If I'm reading this correctly, #3557 PR - Has a dependency on this to fix #3197 @minwoox @jcchavezs

codefromthecrypt · 2023-12-04T00:18:53Z

currently looking at renovating this one

The `HttpResponse` was sent before the request are fully sent. So the request was aborted after getting the response. We should change to send the response after the request is fully received. - close openzipkin#3197 Signed-off-by: Adrian Cole <adrian@tetrate.io>

codefromthecrypt · 2023-12-04T03:08:29Z

ok 🤞 salvaging a three year old change!

codefromthecrypt · 2023-12-04T03:10:14Z

...n-server/src/test/java/zipkin2/server/internal/elasticsearch/ITElasticsearchHealthCheck.java

@@ -47,26 +50,36 @@ public class ITElasticsearchHealthCheck {

  static final SettableHealthChecker server1Health = new SettableHealthChecker(true);

-  static {
-    // Gives better context when there's an exception such as AbortedStreamException
-    System.setProperty("com.linecorp.armeria.verboseExceptions", "always");


already in log config, but also we weren't getting exceptions ;)

codefromthecrypt reviewed Oct 15, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/ElasticsearchStorage.java Outdated Show resolved Hide resolved

minwoox force-pushed the fix_flaky branch from 734ec3c to 79c9461 Compare October 15, 2020 14:15

codefromthecrypt approved these changes Oct 15, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java Outdated Show resolved Hide resolved

...n-server/src/test/java/zipkin2/server/internal/elasticsearch/ITElasticsearchHealthCheck.java Outdated Show resolved Hide resolved

codefromthecrypt reviewed Oct 16, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java Outdated Show resolved Hide resolved

codefromthecrypt reviewed Oct 16, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java Outdated Show resolved Hide resolved

codefromthecrypt reviewed Oct 16, 2020

View reviewed changes

benchmarks/pom.xml Outdated Show resolved Hide resolved

anuraaga reviewed Oct 16, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/client/HttpCall.java Outdated Show resolved Hide resolved

minwoox force-pushed the fix_flaky branch from 9db0c48 to 894e62c Compare October 16, 2020 06:58

anuraaga approved these changes Oct 16, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java Show resolved Hide resolved

codefromthecrypt reviewed Oct 16, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java Outdated Show resolved Hide resolved

codefromthecrypt mentioned this pull request Oct 17, 2020

Refactors Cassandra queries so they are cheaper and easier to migrate #3243

Merged

minwoox mentioned this pull request Oct 19, 2020

Add a document for streaming request using backpressure line/armeria#3119

Open

minwoox force-pushed the fix_flaky branch from 7dea8c1 to 378592c Compare October 22, 2020 02:43

minwoox changed the title ~~Fix flaky test in ITElasticsearchHealthCheck~~ Update Armeria to 1.2.0 and Netty to 4.1.53 Oct 22, 2020

minwoox commented Oct 22, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java Outdated Show resolved Hide resolved

codefromthecrypt reviewed Oct 22, 2020

View reviewed changes

zipkin-storage/elasticsearch/src/main/java/zipkin2/elasticsearch/internal/BulkCallBuilder.java Outdated Show resolved Hide resolved

anuraaga approved these changes Oct 22, 2020

View reviewed changes

minwoox force-pushed the fix_flaky branch from d5e174a to 0c59872 Compare October 22, 2020 04:58

minwoox force-pushed the fix_flaky branch from 7578e5a to d12b268 Compare October 22, 2020 05:59

codefromthecrypt reviewed Oct 22, 2020

View reviewed changes

...-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/internal/client/HttpCallTest.java Outdated Show resolved Hide resolved

minwoox changed the title ~~Update Armeria to 1.2.0 and Netty to 4.1.53~~ Use back pressure for streaming request Oct 22, 2020

minwoox changed the title ~~Use back pressure for streaming request~~ Use backpressure for streaming request Oct 22, 2020

codefromthecrypt force-pushed the fix_flaky branch from d12b268 to 561de8a Compare December 4, 2023 03:06

codefromthecrypt changed the title ~~Use backpressure for streaming request~~ elasticsearch: use backpressure to resolve streaming request flakes Dec 4, 2023

codefromthecrypt changed the title ~~elasticsearch: use backpressure to resolve streaming request flakes~~ elasticsearch: use backpressure to resolve streaming request flakes Dec 4, 2023

codefromthecrypt requested a review from anuraaga December 4, 2023 03:07

codefromthecrypt reviewed Dec 4, 2023

View reviewed changes

anuraaga approved these changes Dec 4, 2023

View reviewed changes

codefromthecrypt merged commit 81b8f16 into openzipkin:master Dec 4, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

elasticsearch: use backpressure to resolve streaming request flakes #3236

elasticsearch: use backpressure to resolve streaming request flakes #3236

minwoox commented Oct 15, 2020 •

edited by codefromthecrypt

Loading

minwoox commented Oct 15, 2020

minwoox commented Oct 15, 2020

codefromthecrypt left a comment

minwoox commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

minwoox commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

minwoox commented Oct 16, 2020

minwoox commented Oct 22, 2020

codefromthecrypt left a comment

minwoox commented Oct 22, 2020

codefromthecrypt commented Oct 22, 2020

darkmastermindz commented Aug 17, 2023

codefromthecrypt commented Dec 4, 2023

codefromthecrypt commented Dec 4, 2023

codefromthecrypt Dec 4, 2023

elasticsearch: use backpressure to resolve streaming request flakes #3236

elasticsearch: use backpressure to resolve streaming request flakes #3236

Conversation

minwoox commented Oct 15, 2020 • edited by codefromthecrypt Loading

minwoox commented Oct 15, 2020

minwoox commented Oct 15, 2020

codefromthecrypt left a comment

Choose a reason for hiding this comment

minwoox commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

minwoox commented Oct 16, 2020

codefromthecrypt commented Oct 16, 2020

minwoox commented Oct 16, 2020

minwoox commented Oct 22, 2020

codefromthecrypt left a comment

Choose a reason for hiding this comment

minwoox commented Oct 22, 2020

codefromthecrypt commented Oct 22, 2020

darkmastermindz commented Aug 17, 2023

codefromthecrypt commented Dec 4, 2023

codefromthecrypt commented Dec 4, 2023

codefromthecrypt Dec 4, 2023

Choose a reason for hiding this comment

minwoox commented Oct 15, 2020 •

edited by codefromthecrypt

Loading