Skip to content
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

Jump to bottom

Ilford updates #1533

Closed
wants to merge 4 commits into from
Closed

Ilford updates #1533

wants to merge 4 commits into from

Conversation

spencergibb
Copy link
Member

Bumps versions to 3.0.0 and removes ribbon, zuul, hystrix and s-c-aws.

Not sure if tests pass (probably don't), but I'd want someone else to finish the work.

@spencergibb spencergibb added this to In progress in 2020.0.0-M1 via automation Jan 22, 2020
@spencergibb spencergibb added this to the 3.0.0.M1 milestone Jan 22, 2020
@codefromthecrypt codefromthecrypt self-assigned this Jan 23, 2020
@codefromthecrypt
Copy link

I can take this over.. I'll probably rebase etc. thanks for the start @spencergibb !

@spencergibb
Copy link
Member Author

Thanks! It compiles. Welcome back!

@codefromthecrypt
Copy link

In ribbon version, WebClientDiscoveryExceptionTests.testFeignInterfaceWithException() raises com.netflix.client.ClientException: Load balancer does not have available server for client: exceptionservice Without ribbon, we get something slightly different: there's no registration for the service "exceptionservice", and feign actually returns a synthetic 503 instead of raising an exception.

In a separate PR, I think probably TraceFeignBlockingLoadBalancerClient should catch this 503 (even in the 2.2 line), so that when there's nothing registered there's at least a span suggesting what happened. We could either look at the 503 alone, or poke at response.request() to see if there were trace headers (there aren't when feign returns the synthetic response). we probably need a new test.

To make WebClientDiscoveryExceptionTests.testFeignInterfaceWithException(), which doesn't have ribbon now, to fail.. somehow we need to register "exceptionservice" I guess, and assume that if it were and no servers were there, the impl would throw? That or maybe it shouldn't really throw, and that's just a legacy.. that 503 is fine?

cc @OlgaMaciaszek for commentary/advise as I'm not sure intent vs behavior here

OlgaMaciaszek
OlgaMaciaszek reviewed Feb 3, 2020
View reviewed changes
Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Looks good, but have added a few comments/ questions.

...framework/cloud/sleuth/instrument/web/client/feign/TraceFeignBlockingLoadBalancerClient.java
&& ((ClientException) e.getCause())
.getErrorType() == ClientException.ErrorType.GENERAL) {
if (e instanceof IOException/*
* || e.getCause() != null && e.getCause()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we commenting this out?

.../test/java/org/springframework/cloud/sleuth/annotation/SleuthSpanCreatorAspectFluxTests.java
@@ -27,7 +27,7 @@
import brave.Tracer;
import brave.propagation.TraceContext;
import brave.sampler.Sampler;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang.StringUtils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why go back to an older version of the lib?

.../java/org/springframework/cloud/sleuth/instrument/web/TraceRestTemplateInterceptorTests.java
@@ -29,7 +29,7 @@
import brave.propagation.ThreadLocalCurrentTraceContext;
import brave.sampler.Sampler;
import brave.spring.web.TracingClientHttpRequestInterceptor;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang.StringUtils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change from lang3 to lang?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a transitive dependency. I don't think we should be using commons lang at all.

...ud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java
@@ -91,7 +84,7 @@ public void should_reuse_custom_feign_client() {
@Test
public void my_client_called() {
this.myNameRemote.get();
then(this.myClient.wasCalled()).isTrue();
// then(this.myClient.wasCalled()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add TODOs to add a client verification?

...ent/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java
@@ -89,10 +82,10 @@ public void open() {
public void should_reuse_custom_feign_client() {
String response = this.myNameRemote.get();

then(this.myClient.wasCalled()).isTrue();
// then(this.myClient.wasCalled()).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add TODOs to add client verification?

@codefromthecrypt codefromthecrypt removed their assignment Feb 5, 2020
@codefromthecrypt
Copy link

taking self off while I focus on reactor. I don't want to block others in case they want to chip in. we are pro non-blocking

@marcingrzejszczak
Copy link
Contributor

I'm taking this over

@marcingrzejszczak marcingrzejszczak self-assigned this Feb 10, 2020
@marcingrzejszczak
Copy link
Contributor

Done via 3813cf9

2020.0.0-M1 automation moved this from In progress to Done Feb 12, 2020
@codefromthecrypt codefromthecrypt deleted the ilford-updates branch May 26, 2020 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlgaMaciaszek OlgaMaciaszek OlgaMaciaszek left review comments

@marcingrzejszczak marcingrzejszczak

@codefromthecrypt codefromthecrypt

Assignees

@marcingrzejszczak marcingrzejszczak

Labels
task
Projects
No open projects
2020.0.0-M1
  
Done
Milestone
3.0.0.M1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@spencergibb @codefromthecrypt @marcingrzejszczak @OlgaMaciaszek