jolshan commented on code in PR #16443:
URL: https://github.com/apache/kafka/pull/16443#discussion_r1775907028
##########
metadata/src/main/java/org/apache/kafka/controller/FeatureControlManager.java:
##########
@@ -174,15 +175,27 @@ ControllerResult<Map<String, ApiError>> updateFeatures(
Map<String, FeatureUpdate.UpgradeType> upgradeTypes,
boolean validateOnly
) {
+ boolean updateFailed = false;
TreeMap<String, ApiError> results = new TreeMap<>();
List<ApiMessageAndVersion> records =
BoundedList.newArrayBacked(MAX_RECORDS_PER_USER_OP);
+
+ Map<String, Short> proposedUpdatedVersions = new HashMap<>();
+ finalizedVersions.forEach(proposedUpdatedVersions::put);
+ proposedUpdatedVersions.put(MetadataVersion.FEATURE_NAME,
metadataVersion.get().featureLevel());
+ updates.forEach(proposedUpdatedVersions::put);
+
for (Entry<String, Short> entry : updates.entrySet()) {
- results.put(entry.getKey(), updateFeature(entry.getKey(),
entry.getValue(),
- upgradeTypes.getOrDefault(entry.getKey(),
FeatureUpdate.UpgradeType.UPGRADE), records));
+ ApiError error = updateFeature(entry.getKey(), entry.getValue(),
+ upgradeTypes.getOrDefault(entry.getKey(),
FeatureUpdate.UpgradeType.UPGRADE), records, proposedUpdatedVersions);
+ results.put(entry.getKey(), error);
+ if (!error.error().equals(Errors.NONE)) {
+ updateFailed = true;
+ break;
+ }
}
- if (validateOnly) {
+ if (validateOnly || updateFailed) {
Review Comment:
Right. This makes sense. I think the part I was not sure about was the
return type of ControllerResult. I think the approach I am taking right now is
to return ControllerResult with a single entry containing the feature with the
error. In QuorumController, I convert this to the top level error to return.
Ie.
```
if (!error.error().equals(Errors.NONE)) {
return ControllerResult.of(Collections.emptyList(),
Collections.singletonMap(entry.getKey(), error));
}
```
```
if (errorEntry.isPresent()) {
String errorFeatureName = errorEntry.get().getKey();
ApiError topError = errorEntry.get().getValue();
String errorString = errorFeatureName + ":" +
topError.error().exceptionName() + " (" + topError.message() + ")";
responseData.setErrorCode(topError.error().code());
responseData.setErrorMessage("The update failed for all
features since the following feature had an error: " + errorString);
```
thinking about ways to clean up how we find the one error. Will update again
after lunch.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
