-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: handle errors in xds_client #3658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the tests yet.
} | ||
} | ||
// When LDS resource is removed, we don't delete corresponding RDS cached | ||
// data. The RDS watch will be canceled, and cache is removed when the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/cache/cache entry/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
// TODO: remove item from cache and remove corresponding RDS cached data. | ||
for name := range c.ldsCache { | ||
if _, ok := d[name]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a better variable name for d
whose scope has now gotten bigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to updates
} | ||
} | ||
// When CDS resource is removed, we don't delete corresponding EDS cached | ||
// data. The EDS watch will be canceled, and cache is removed when the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here.
s/cache/cache entry/
And better name for d
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
|
||
// TODO: remove item from cache and remove corresponding RDS cached data. | ||
for name := range c.ldsCache { | ||
if _, ok := d[name]; !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to updates
} | ||
} | ||
// When LDS resource is removed, we don't delete corresponding RDS cached | ||
// data. The RDS watch will be canceled, and cache is removed when the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
} | ||
// When CDS resource is removed, we don't delete corresponding EDS cached | ||
// data. The EDS watch will be canceled, and cache is removed when the last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
xds_client
test update because it was expecting no update when resource is removed
test cleanup to apply timeout to channels