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

xds: propagate bootstrap error to grpc.Dial #3330

Merged
merged 3 commits into from
Jan 23, 2020

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Jan 22, 2020

This change is Reviewable

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @menghanl)


xds/internal/balancer/edsbalancer/xds_client_wrapper.go, line 137 at r1 (raw file):

		clientConfig = &bootstrap.Config{
			BalancerName: config.BalancerName,
		}

Nit: Fold into a single line since we are setting only one field?


xds/internal/balancer/edsbalancer/xds_client_wrapper.go, line 141 at r1 (raw file):

	} else if clientConfig.BalancerName == "" {
		clientConfig.BalancerName = config.BalancerName
	}

Do we need the else, given that NewConfig has been updated to return an error when BalancerName is empty?


xds/internal/client/bootstrap/bootstrap_test.go, line 247 at r1 (raw file):

Quoted 9 lines of code…
			if test.wantError {
				if err == nil {
					t.Fatalf("wantError: %v, got error %v", test.wantError, err)
				}
				return
			}
			if err != nil {
				t.Fatalf("unexpected error %v", err)
			}

Could this be handled with a single conditional statement like:

if (err != nil) != test.wantErr {
  t.Fatalf("NewConfig() returned err: %v, wantErr: %v", err, test.wantErr)
}

@easwars easwars assigned menghanl and unassigned easwars Jan 22, 2020
Copy link
Contributor Author

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @easwars)


xds/internal/balancer/edsbalancer/xds_client_wrapper.go, line 137 at r1 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…
		clientConfig = &bootstrap.Config{
			BalancerName: config.BalancerName,
		}

Nit: Fold into a single line since we are setting only one field?

Done


xds/internal/balancer/edsbalancer/xds_client_wrapper.go, line 141 at r1 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…
	} else if clientConfig.BalancerName == "" {
		clientConfig.BalancerName = config.BalancerName
	}

Do we need the else, given that NewConfig has been updated to return an error when BalancerName is empty?

Done. Needed to make some changes to tests to get BalancerName (which is being removed in another PR...).


xds/internal/client/bootstrap/bootstrap_test.go, line 247 at r1 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…
			if test.wantError {
				if err == nil {
					t.Fatalf("wantError: %v, got error %v", test.wantError, err)
				}
				return
			}
			if err != nil {
				t.Fatalf("unexpected error %v", err)
			}

Could this be handled with a single conditional statement like:

if (err != nil) != test.wantErr {
  t.Fatalf("NewConfig() returned err: %v, wantErr: %v", err, test.wantErr)
}

I need it to return in case of errors, otherwise the following code will nil panic.

I reversed the order, to make it look slightly better. But there are still two if statements

Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@menghanl menghanl merged commit 1f66bc9 into grpc:master Jan 23, 2020
@menghanl menghanl deleted the xds_error_cases branch January 23, 2020 20:46
@menghanl menghanl changed the title xds: propogate bootstrap error to grpc.Dial xds: propagate bootstrap error to grpc.Dial Feb 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants