You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/22 22:55:54 UTC
[GitHub] [kafka] hachikuji opened a new pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
hachikuji opened a new pull request #9781:
URL: https://github.com/apache/kafka/pull/9781
The current `getErrorResponse` sets all of the feature errors, but does not set a top-level error. It seems like the whole point of having the top-level error is so that it could be used in cases like this.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
hachikuji commented on pull request #9781:
URL: https://github.com/apache/kafka/pull/9781#issuecomment-752188121
@chia7712 Since this is a new api, I think we are free to choose its error semantics. I agree that there is some inconsistency between different APIs, but we are often stuck with that because a top-level error code didn't come until later (e.g. see `FetchResponse`). The benefit of having a top-level error is that we do not need to propagate it. See `AlterIsr` for a similar example. Also note the handling logic in `KafkaApis.handleUpdateFeatures` which already relies on this approach: https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/KafkaApis.scala#L3222.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9781:
URL: https://github.com/apache/kafka/pull/9781#discussion_r547605207
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/UpdateFeaturesRequest.java
##########
@@ -56,19 +54,11 @@ public UpdateFeaturesRequest(UpdateFeaturesRequestData data, short version) {
@Override
public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
- final ApiError apiError = ApiError.fromThrowable(e);
- final UpdatableFeatureResultCollection results = new UpdatableFeatureResultCollection();
- for (FeatureUpdateKey update : this.data.featureUpdates().valuesSet()) {
- final UpdatableFeatureResult result = new UpdatableFeatureResult()
- .setFeature(update.feature())
- .setErrorCode(apiError.error().code())
- .setErrorMessage(apiError.message());
- results.add(result);
- }
- final UpdateFeaturesResponseData responseData = new UpdateFeaturesResponseData()
- .setThrottleTimeMs(throttleTimeMs)
- .setResults(results);
- return new UpdateFeaturesResponse(responseData);
+ return UpdateFeaturesResponse.createWithErrors(
+ ApiError.fromThrowable(e),
+ Collections.emptyMap(),
Review comment:
If we want set only top-level error, we have to make ```UpdateFeaturesResponse#errors``` use top-level error as well. Otherwise, the top-level error can't be propagated correctly.
see https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/requests/UpdateFeaturesResponse.java#L48
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kowshik commented on a change in pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #9781:
URL: https://github.com/apache/kafka/pull/9781#discussion_r547557452
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/UpdateFeaturesRequest.java
##########
@@ -57,18 +55,11 @@ public UpdateFeaturesRequest(UpdateFeaturesRequestData data, short version) {
@Override
public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
final ApiError apiError = ApiError.fromThrowable(e);
- final UpdatableFeatureResultCollection results = new UpdatableFeatureResultCollection();
- for (FeatureUpdateKey update : this.data.featureUpdates().valuesSet()) {
- final UpdatableFeatureResult result = new UpdatableFeatureResult()
- .setFeature(update.feature())
- .setErrorCode(apiError.error().code())
- .setErrorMessage(apiError.message());
- results.add(result);
- }
- final UpdateFeaturesResponseData responseData = new UpdateFeaturesResponseData()
- .setThrottleTimeMs(throttleTimeMs)
- .setResults(results);
- return new UpdateFeaturesResponse(responseData);
+ return UpdateFeaturesResponse.createWithErrors(
+ apiError,
Review comment:
You could inline `ApiError.fromThrowable(e)` here, and eliminate the `apiError` local variable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9781:
URL: https://github.com/apache/kafka/pull/9781#discussion_r549580987
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/UpdateFeaturesResponse.java
##########
@@ -45,16 +44,18 @@ public UpdateFeaturesResponse(UpdateFeaturesResponseData data) {
this.data = data;
}
- public Map<String, ApiError> errors() {
- return data.results().valuesSet().stream().collect(
- Collectors.toMap(
- result -> result.feature(),
- result -> new ApiError(Errors.forCode(result.errorCode()), result.errorMessage())));
+ public ApiError topLevelError() {
+ return new ApiError(Errors.forCode(data.errorCode()), data.errorMessage());
}
@Override
public Map<Errors, Integer> errorCounts() {
- return apiErrorCounts(errors());
+ Map<Errors, Integer> errorCounts = new HashMap<>();
+ updateErrorCounts(errorCounts, Errors.forCode(data.errorCode()));
+ for (UpdatableFeatureResult result : data.results().valuesSet()) {
Review comment:
Is ```valuesSet``` necessary?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9781:
URL: https://github.com/apache/kafka/pull/9781#discussion_r549426896
##########
File path: clients/src/main/java/org/apache/kafka/common/requests/UpdateFeaturesRequest.java
##########
@@ -56,19 +54,11 @@ public UpdateFeaturesRequest(UpdateFeaturesRequestData data, short version) {
@Override
public AbstractResponse getErrorResponse(int throttleTimeMs, Throwable e) {
- final ApiError apiError = ApiError.fromThrowable(e);
- final UpdatableFeatureResultCollection results = new UpdatableFeatureResultCollection();
- for (FeatureUpdateKey update : this.data.featureUpdates().valuesSet()) {
- final UpdatableFeatureResult result = new UpdatableFeatureResult()
- .setFeature(update.feature())
- .setErrorCode(apiError.error().code())
- .setErrorMessage(apiError.message());
- results.add(result);
- }
- final UpdateFeaturesResponseData responseData = new UpdateFeaturesResponseData()
- .setThrottleTimeMs(throttleTimeMs)
- .setResults(results);
- return new UpdateFeaturesResponse(responseData);
+ return UpdateFeaturesResponse.createWithErrors(
+ ApiError.fromThrowable(e),
+ Collections.emptyMap(),
Review comment:
@chia7712 Good catch. Pushed an update with some test cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #9781: MINOR: Use top-level error in `UpdateFeaturesRequest.getErrorResponse`
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9781:
URL: https://github.com/apache/kafka/pull/9781
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org