exceptionfactory commented on code in PR #10619:
URL: https://github.com/apache/nifi/pull/10619#discussion_r2604404125
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -154,64 +154,28 @@ public void shutdown() {
@Override
public void setState(final Map<String, String> state, final String
componentId) throws IOException {
try {
- final ConfigMap configMap = createConfigMapBuilder(state,
componentId).build();
- Resource<ConfigMap> configMapResource =
kubernetesClient.configMaps().resource(configMap);
- final String configMapName = configMap.getMetadata().getName();
-
- ConfigMap configMapCreated = null;
+ final ConfigMap configMap = createConfigMapBuilder(state,
componentId, Optional.empty()).build();
+ ConfigMap configMapResult = null;
- // Attempt to create or update, up to 3 times. We expect that we
will update more frequently than create
- // so we first attempt to update. If we get back a 404, then we
create it.
- boolean create = false;
+ // Attempt to create or update, up to 3 times
for (int attempt = 0; attempt < MAX_UPDATE_ATTEMPTS; attempt++) {
try {
- if (create) {
- configMapCreated = configMapResource.create();
+ final ConfigMap existingConfigMap =
kubernetesClient.configMaps().resource(configMap).get();
+ if (existingConfigMap == null) {
+ configMapResult =
kubernetesClient.configMaps().resource(configMap).create();
} else {
- configMapCreated = configMapResource.update();
+ existingConfigMap.setData(configMap.getData());
+ configMapResult =
kubernetesClient.configMaps().resource(existingConfigMap).update();
}
-
break;
} catch (final KubernetesClientException e) {
final int returnCode = e.getCode();
- if (returnCode == HttpURLConnection.HTTP_NOT_FOUND) {
- // A 404 return code indicates that we need to create
the resource instead of update it.
- // Now, we will attempt to create the resource instead
of update it, so we'll reset the attempt counter.
- attempt = 0;
- create = true;
- continue;
- }
-
- if (returnCode == HttpURLConnection.HTTP_CONFLICT) {
- logger.debug("Update conflict detected when setting
state for Component ID [{}]. Attempt {} of {}.", componentId, attempt + 1,
MAX_UPDATE_ATTEMPTS);
-
- if (attempt < MAX_UPDATE_ATTEMPTS - 1) {
- final ConfigMap latestConfigMap =
kubernetesClient.configMaps()
- .inNamespace(namespace)
- .withName(configMapName)
- .get();
-
- if (latestConfigMap != null) {
- final ObjectMeta latestMetadata =
latestConfigMap.getMetadata();
- final String latestResourceVersion =
latestMetadata != null ? latestMetadata.getResourceVersion() : null;
-
- if (latestResourceVersion != null) {
-
configMap.getMetadata().setResourceVersion(latestResourceVersion);
- configMapResource =
kubernetesClient.configMaps().resource(configMap);
- logger.debug("Retrying state update for
Component ID [{}] with resource version [{}]", componentId,
latestResourceVersion);
- continue;
- }
- }
- }
-
- throw e;
- }
-
- if (returnCode >= 500) {
- // Server-side error. We should retry, up to some
number of attempts.
+ if (returnCode == HttpURLConnection.HTTP_CONFLICT ||
returnCode >= 500) {
+ // Conflict or Server-side error. We should retry, up
to some number of attempts.
if (attempt == MAX_UPDATE_ATTEMPTS - 1) {
throw e;
}
+ logger.warn("Failed to update state for Component ID
[{}]. Attempt {} of {}.", componentId, attempt + 1, MAX_UPDATE_ATTEMPTS, e);
Review Comment:
Should this be a warning? It if it uncommon, that a warning seems useful. If
it is common, an `INFO` message seems better.
```suggestion
logger.warn("Failed to update state for Component ID
[{}] on attempt {} of {}", componentId, attempt + 1, MAX_UPDATE_ATTEMPTS, e);
```
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -154,64 +154,28 @@ public void shutdown() {
@Override
public void setState(final Map<String, String> state, final String
componentId) throws IOException {
try {
- final ConfigMap configMap = createConfigMapBuilder(state,
componentId).build();
- Resource<ConfigMap> configMapResource =
kubernetesClient.configMaps().resource(configMap);
- final String configMapName = configMap.getMetadata().getName();
-
- ConfigMap configMapCreated = null;
+ final ConfigMap configMap = createConfigMapBuilder(state,
componentId, Optional.empty()).build();
+ ConfigMap configMapResult = null;
- // Attempt to create or update, up to 3 times. We expect that we
will update more frequently than create
- // so we first attempt to update. If we get back a 404, then we
create it.
- boolean create = false;
+ // Attempt to create or update, up to 3 times
for (int attempt = 0; attempt < MAX_UPDATE_ATTEMPTS; attempt++) {
try {
- if (create) {
- configMapCreated = configMapResource.create();
+ final ConfigMap existingConfigMap =
kubernetesClient.configMaps().resource(configMap).get();
+ if (existingConfigMap == null) {
+ configMapResult =
kubernetesClient.configMaps().resource(configMap).create();
} else {
- configMapCreated = configMapResource.update();
+ existingConfigMap.setData(configMap.getData());
+ configMapResult =
kubernetesClient.configMaps().resource(existingConfigMap).update();
}
-
break;
} catch (final KubernetesClientException e) {
final int returnCode = e.getCode();
- if (returnCode == HttpURLConnection.HTTP_NOT_FOUND) {
- // A 404 return code indicates that we need to create
the resource instead of update it.
- // Now, we will attempt to create the resource instead
of update it, so we'll reset the attempt counter.
- attempt = 0;
- create = true;
- continue;
- }
-
- if (returnCode == HttpURLConnection.HTTP_CONFLICT) {
- logger.debug("Update conflict detected when setting
state for Component ID [{}]. Attempt {} of {}.", componentId, attempt + 1,
MAX_UPDATE_ATTEMPTS);
-
- if (attempt < MAX_UPDATE_ATTEMPTS - 1) {
- final ConfigMap latestConfigMap =
kubernetesClient.configMaps()
- .inNamespace(namespace)
- .withName(configMapName)
- .get();
-
- if (latestConfigMap != null) {
- final ObjectMeta latestMetadata =
latestConfigMap.getMetadata();
- final String latestResourceVersion =
latestMetadata != null ? latestMetadata.getResourceVersion() : null;
-
- if (latestResourceVersion != null) {
-
configMap.getMetadata().setResourceVersion(latestResourceVersion);
- configMapResource =
kubernetesClient.configMaps().resource(configMap);
- logger.debug("Retrying state update for
Component ID [{}] with resource version [{}]", componentId,
latestResourceVersion);
- continue;
- }
- }
- }
-
- throw e;
- }
-
- if (returnCode >= 500) {
- // Server-side error. We should retry, up to some
number of attempts.
+ if (returnCode == HttpURLConnection.HTTP_CONFLICT ||
returnCode >= 500) {
Review Comment:
Recommend replacing `500` with `HttpURLConnection.HTTP_INTERNAL_ERROR` to
align with the use of `HTTP_CONFLICT`.
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -411,15 +381,28 @@ private Resource<ConfigMap> configMapResource(final
String componentId) {
return
kubernetesClient.configMaps().inNamespace(namespace).withName(name);
}
- private ConfigMapBuilder createConfigMapBuilder(final Map<String, String>
state, final String componentId) {
- final Map<String, String> encodedData = getEncodedMap(state);
+ private ConfigMapBuilder createConfigMapBuilder(final Map<String, String>
state, final String componentId, final Optional<ObjectMeta> existingMetadata) {
final String name = getConfigMapName(componentId);
- return new ConfigMapBuilder()
+
+ final ConfigMapBuilder configMapBuilder;
+ if (existingMetadata.isPresent()) {
+ if (!namespace.equals(existingMetadata.get().getNamespace())) {
+ throw new IllegalArgumentException("Expected existing
ConfigMap namespace [%s], but was [%s]".formatted(namespace,
existingMetadata.get().getNamespace()));
+ }
+ if (!name.equals(existingMetadata.get().getName())) {
+ throw new IllegalArgumentException("Expected existing
ConfigMap name [%s], but was [%s]".formatted(name,
existingMetadata.get().getName()));
+ }
Review Comment:
Is very likely to happen given that this is framework-level component?
Perhaps rewriting this a bit to declare the `ObjectMeta` instance on one line,
and then checking the positive conditions of `namespace` and `name` matching.
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -411,15 +381,28 @@ private Resource<ConfigMap> configMapResource(final
String componentId) {
return
kubernetesClient.configMaps().inNamespace(namespace).withName(name);
}
- private ConfigMapBuilder createConfigMapBuilder(final Map<String, String>
state, final String componentId) {
- final Map<String, String> encodedData = getEncodedMap(state);
+ private ConfigMapBuilder createConfigMapBuilder(final Map<String, String>
state, final String componentId, final Optional<ObjectMeta> existingMetadata) {
Review Comment:
Instead of passing the `Optional` to this `private` method, it looks like
the `ObjectMeta` itself could be passed and checked for `null`.
##########
nifi-framework-bundle/nifi-framework-extensions/nifi-framework-kubernetes-bundle/nifi-framework-kubernetes-state-provider/src/main/java/org/apache/nifi/kubernetes/state/provider/KubernetesConfigMapStateProvider.java:
##########
@@ -272,7 +237,12 @@ public StateMap getState(final String componentId) throws
IOException {
*/
@Override
public boolean replace(final StateMap currentState, final Map<String,
String> state, final String componentId) throws IOException {
- final ConfigMapBuilder configMapBuilder =
createConfigMapBuilder(state, componentId);
+ if (!(currentState instanceof StandardStateMap)) {
+ throw new IllegalStateException("Current state is not an instance
of StandardStateMap");
+ }
+
+ final Optional<ObjectMeta> existingMetadata = ((StandardStateMap)
currentState).getConfigMapMetadata();
Review Comment:
The casting is a bit verbose as written. Instead, what do you think about
using pattern variables as `if (currentState instanceof StandardStateMap
standardStateMap) { ... } else { throw new IllegalStateException() }`
--
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]
