exceptionfactory commented on code in PR #10238: URL: https://github.com/apache/nifi/pull/10238#discussion_r2328418712
########## nifi-system-tests/nifi-system-test-extensions-bundle/nifi-system-test-extensions/src/main/java/org/apache/nifi/processors/tests/system/MultiKeyStateNotDroppable.java: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.processors.tests.system; + +import org.apache.nifi.annotation.behavior.PrimaryNodeOnly; +import org.apache.nifi.annotation.behavior.Stateful; +import org.apache.nifi.annotation.configuration.DefaultSchedule; +import org.apache.nifi.components.state.Scope; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +@PrimaryNodeOnly +@DefaultSchedule(period = "10 mins") +@Stateful(scopes = Scope.CLUSTER, description = "Stores three counters in state", dropStateKeySupported = false) +public class MultiKeyStateNotDroppable extends AbstractProcessor { + public static final Relationship REL_SUCCESS = new Relationship.Builder() + .name("success") + .autoTerminateDefault(true) + .build(); + + @Override + public Set<Relationship> getRelationships() { + return new HashSet<>(Collections.singletonList(REL_SUCCESS)); Review Comment: ```suggestion return Set.of(REL_SUCCESS); ``` ########## nifi-system-tests/nifi-system-test-extensions-bundle/nifi-system-test-extensions/src/main/java/org/apache/nifi/processors/tests/system/MultiKeyState.java: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.processors.tests.system; + +import org.apache.nifi.annotation.behavior.PrimaryNodeOnly; +import org.apache.nifi.annotation.behavior.Stateful; +import org.apache.nifi.annotation.configuration.DefaultSchedule; +import org.apache.nifi.components.state.Scope; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; + +import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +@PrimaryNodeOnly +@DefaultSchedule(period = "10 mins") +@Stateful(scopes = Scope.CLUSTER, description = "Stores three counters in state", dropStateKeySupported = true) +public class MultiKeyState extends AbstractProcessor { + public static final Relationship REL_SUCCESS = new Relationship.Builder() + .name("success") + .autoTerminateDefault(true) + .build(); + + @Override + public Set<Relationship> getRelationships() { + return new HashSet<>(Collections.singletonList(REL_SUCCESS)); Review Comment: ```suggestion return Set.of(REL_SUCCESS); ``` ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessorResource.java: ########## @@ -489,7 +489,7 @@ public Response getState( * @throws InterruptedException if interrupted */ @POST - @Consumes(MediaType.WILDCARD) + @Consumes(MediaType.APPLICATION_JSON) Review Comment: Following the pattern of other Resource methods, it looks like this should support both wildcard and JSON, correct? ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardComponentStateDAO.java: ########## @@ -42,38 +46,98 @@ private StateMap getState(final String componentId, final Scope scope) { try { final StateManager manager = stateManagerProvider.getStateManager(componentId); if (manager == null) { - throw new ResourceNotFoundException(String.format("State for the specified component %s could not be found.", componentId)); + throw new ResourceNotFoundException("State for the specified component %s could not be found.".formatted(componentId)); } return manager.getState(scope); } catch (final IOException ioe) { - throw new IllegalStateException(String.format("Unable to get the state for the specified component %s: %s", componentId, ioe), ioe); + throw new IllegalStateException("Unable to get the state for the specified component %s: %s".formatted(componentId, ioe), ioe); } } - private void clearState(final String componentId) { + private void clearState(final String componentId, ComponentStateDTO componentStateDTO) { try { final StateManager manager = stateManagerProvider.getStateManager(componentId); if (manager == null) { - throw new ResourceNotFoundException(String.format("State for the specified component %s could not be found.", componentId)); + throw new ResourceNotFoundException("State for the specified component %s could not be found.".formatted(componentId)); } - // clear both state's at the same time - manager.clear(Scope.CLUSTER); - manager.clear(Scope.LOCAL); + if (componentStateDTO == null) { + // clear both state's at the same time + manager.clear(Scope.CLUSTER); + manager.clear(Scope.LOCAL); + } else if (manager.getState(Scope.LOCAL) != null && !manager.getState(Scope.LOCAL).toMap().isEmpty() && !stateManagerProvider.isClusterProviderEnabled()) { Review Comment: The conditional in each of these clauses is a bit hard to follow. It seems like nesting branches for `componentStateDTO` not null would support a single check for the existing of local state, and then subsequent checks would be easier to follow. ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/ControllerServiceDAO.java: ########## @@ -150,6 +151,7 @@ public interface ControllerServiceDAO { * Clears the state of the specified controller service. * * @param controllerServiceId controller service id + * @param componentStateDTO state of the controller service */ - void clearState(String controllerServiceId); + void clearState(final String controllerServiceId, final ComponentStateDTO componentStateDTO); Review Comment: Minor, but interface method arguments do not need `final` ########## nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardComponentStateDAO.java: ########## @@ -42,38 +46,98 @@ private StateMap getState(final String componentId, final Scope scope) { try { final StateManager manager = stateManagerProvider.getStateManager(componentId); if (manager == null) { - throw new ResourceNotFoundException(String.format("State for the specified component %s could not be found.", componentId)); + throw new ResourceNotFoundException("State for the specified component %s could not be found.".formatted(componentId)); } return manager.getState(scope); } catch (final IOException ioe) { - throw new IllegalStateException(String.format("Unable to get the state for the specified component %s: %s", componentId, ioe), ioe); + throw new IllegalStateException("Unable to get the state for the specified component %s: %s".formatted(componentId, ioe), ioe); } } - private void clearState(final String componentId) { + private void clearState(final String componentId, ComponentStateDTO componentStateDTO) { try { final StateManager manager = stateManagerProvider.getStateManager(componentId); if (manager == null) { - throw new ResourceNotFoundException(String.format("State for the specified component %s could not be found.", componentId)); + throw new ResourceNotFoundException("State for the specified component %s could not be found.".formatted(componentId)); } - // clear both state's at the same time - manager.clear(Scope.CLUSTER); - manager.clear(Scope.LOCAL); + if (componentStateDTO == null) { + // clear both state's at the same time + manager.clear(Scope.CLUSTER); + manager.clear(Scope.LOCAL); + } else if (manager.getState(Scope.LOCAL) != null && !manager.getState(Scope.LOCAL).toMap().isEmpty() && !stateManagerProvider.isClusterProviderEnabled()) { + // we are in standalone mode, all state is local + if (manager.isStateKeyDropSupported()) { + final Map<String, String> newState = toStateMap(componentStateDTO.getLocalState()); + final Map<String, String> currentState = manager.getState(Scope.LOCAL).toMap(); + + if (hasExactlyOneKeyRemoved(currentState, newState)) { + manager.setState(newState, Scope.LOCAL); + } else { + throw new IllegalStateException("Unable to remove a state key for component %s. Exactly one key removal is supported.".formatted(componentId)); + } + } else { + throw new IllegalStateException("Selective state key removal is not supported for component %s with local state.".formatted(componentId)); + } + } else if (manager.getState(Scope.LOCAL) != null && !manager.getState(Scope.LOCAL).toMap().isEmpty()) { + throw new IllegalStateException("Selective state key removal is not supported for component %s with local state.".formatted(componentId)); + } else if (componentStateDTO.getClusterState() != null && !componentStateDTO.getClusterState().getState().isEmpty()) { + if (manager.isStateKeyDropSupported()) { + final Map<String, String> newState = toStateMap(componentStateDTO.getClusterState()); + final Map<String, String> currentState = manager.getState(Scope.CLUSTER).toMap(); + + if (hasExactlyOneKeyRemoved(currentState, newState)) { + manager.setState(newState, Scope.CLUSTER); + } else { + throw new IllegalStateException("Unable to remove a state key for component %s. Exactly one key removal is supported.".formatted(componentId)); + } + + // we clear local anyway + manager.clear(Scope.LOCAL); + } else { + throw new IllegalStateException("Selective state key removal is not supported for component %s with cluster state.".formatted(componentId)); + } + } else { + // clear both state's at the same time + manager.clear(Scope.CLUSTER); + manager.clear(Scope.LOCAL); + } } catch (final IOException ioe) { - throw new IllegalStateException(String.format("Unable to clear the state for the specified component %s: %s", componentId, ioe), ioe); + throw new IllegalStateException("Unable to clear the state for the specified component %s: %s".formatted(componentId, ioe), ioe); } } + public boolean hasExactlyOneKeyRemoved(Map<String, String> currentState, Map<String, String> newState) { Review Comment: Should this method be `public`? -- 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]
