exceptionfactory commented on code in PR #9837: URL: https://github.com/apache/nifi/pull/9837#discussion_r2027983781
########## nifi-framework-api/src/main/java/org/apache/nifi/action/ActionConverter.java: ########## @@ -0,0 +1,25 @@ +/* + * 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.action; + +/** + * Converter for converting an action to a flow action. Used in the reporting flow actions process. + */ +public interface ActionConverter { Review Comment: With this conversion being more of an implementation detail, I recommend moving this interface out of `nifi-framework-api` and into the component module. ########## nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java: ########## @@ -331,6 +331,9 @@ public class NiFiProperties extends ApplicationProperties { // performance tracking public static final String TRACK_PERFORMANCE_PERCENTAGE = "nifi.performance.tracking.percentage"; + // flow action reporter + public static final String COMPONENT_FLOW_ACTION_REPORTER_IMPLEMENTATION = "nifi.components.flow.action.reporter.implementation"; Review Comment: The `components` element can be removed. As this is a convenience property, it is probably best to avoid adding it to `NiFiProperties` and define it in the appropriate class. ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowActionReporterConfigurationContext.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.action; + +import javax.net.ssl.SSLContext; +import javax.net.ssl.X509TrustManager; +import java.util.Map; + +/** + * A context that will be passed to the reporter in order to obtain configuration. + */ +public interface FlowActionReporterConfigurationContext { + /** + * Retrieves all properties the context currently understands regardless + * of whether a value has been set for them or not. If no value is present + * then its value is null and thus any registered default for the property + * descriptor applies. + * + * @return Map of all properties + */ + Map<String, String> getProperties(); Review Comment: The source of these properties is not clear from the documentation, is this from application properties defined in `nifi.properties`? As it appears to be an empty Map on implementation, it is best to remove it for now. ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowActionReporterConfigurationContext.java: ########## @@ -0,0 +1,48 @@ +/* + * 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.action; + +import javax.net.ssl.SSLContext; +import javax.net.ssl.X509TrustManager; +import java.util.Map; + +/** + * A context that will be passed to the reporter in order to obtain configuration. + */ +public interface FlowActionReporterConfigurationContext { + /** + * Retrieves all properties the context currently understands regardless + * of whether a value has been set for them or not. If no value is present + * then its value is null and thus any registered default for the property + * descriptor applies. + * + * @return Map of all properties + */ + Map<String, String> getProperties(); + + /** + * Retrieves SSLContext if set + * @return SSLContext + */ + SSLContext getSSLContext(); + + /** + * Retrieves the trust manager if set + * @return X509TrustManager + */ + X509TrustManager getTrustManager(); Review Comment: Both of these objects should be wrapped with `Optional` to indicate that the framework may not provide them. ########## nifi-framework-bundle/nifi-framework/nifi-headless-server/src/main/java/org/apache/nifi/headless/HeadlessNiFiServer.java: ########## @@ -107,7 +110,7 @@ public void start() { logger.info("Loading Flow..."); FlowFileEventRepository flowFileEventRepository = new RingBufferEventRepository(5); - AuditService auditService = new HeadlessAuditService(); + AuditService auditService = new ReportableAuditService(new HeadlessAuditService(), new HeadlessFlowActionReporter(), new HeadlessActionConverter()); Review Comment: This change is unnecessary, and actually indicates that the `Headless` implementation classes can be removed. ########## nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/action/ReportableAuditService.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.action; + +import org.apache.nifi.admin.service.AuditService; +import org.apache.nifi.history.History; +import org.apache.nifi.history.HistoryQuery; +import org.apache.nifi.history.PreviousValue; + +import java.util.Collection; +import java.util.Date; +import java.util.List; +import java.util.Map; + +/** + * An {@link AuditService} that reports flow actions to a {@link FlowActionReporter}. + */ +public class ReportableAuditService implements AuditService { Review Comment: This wrapper implementation seems unnecessary. Instead, I recommend wiring the `FlowActionReporter` directly to the `AuditService` implementation or directly to where `addActions` is called to avoid the coupling. ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowActionAttributes.java: ########## @@ -0,0 +1,86 @@ +/* + * 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.action; + +/** + * Flow Action attributes for consistent naming + */ +public interface FlowActionAttributes { + + enum ACTION implements FlowActionAttribute { Review Comment: The nested `enum` approach does not seem like the best approach. Instead, recommend using a single `enum` to enumerate all the attribute names, while retaining the prefixing. ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/action/EmptyFlowAction.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.action; + +import java.util.Map; + +/** + * Empty flow action to ensure no attributes are added to the flow action. + */ +public class EmptyFlowAction implements FlowAction { Review Comment: Recommend moving this under `nifi-headless-server` to indicate that it is not used for standard framework operations. ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/action/HeadlessActionConverter.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.action; + +/** + * Headless implementation of the {@link ActionConverter} interface that converts to an {@link EmptyFlowAction}. + */ +public class HeadlessActionConverter implements ActionConverter { Review Comment: Recommend moving this under `nifi-headless-server` ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowAction.java: ########## @@ -0,0 +1,32 @@ +/* + * 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.action; + +import java.util.Map; + +/** + * An interface that represents an action that can be taken on a flow + */ +public interface FlowAction { + Map<String, String> getAttributes(); + + void putAllAttributes(Map<String, String> properties); + + String getAttribute(String key); + + void putAttribute(String key, String value); Review Comment: These methods should have documentation. As a `FlowAction` is something to be consumed, I recommend simplifying the interface to just `getAttributes()`, the convenience `getAttribute()` method is not necessary. ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowActionReporter.java: ########## @@ -0,0 +1,29 @@ +/* + * 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.action; + +import java.util.Collection; + +public interface FlowActionReporter { + default void onConfigured(FlowActionReporterConfigurationContext context) throws FlowActionReporterCreationException { + } + + void reportFlowActions(Collection<FlowAction> actions); + + default void preDestruction() { + } Review Comment: The class and method should have basic documentation, following other framework interfaces. ########## nifi-framework-api/src/main/java/org/apache/nifi/action/FlowActionReporterCreationException.java: ########## @@ -0,0 +1,39 @@ +/* + * 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.action; + +/** + * An exception that will be thrown if a reporter can not be created. + */ +public class FlowActionReporterCreationException extends RuntimeException { + + public FlowActionReporterCreationException() { + } + + public FlowActionReporterCreationException(String message) { + super(message); + } + + public FlowActionReporterCreationException(String message, Throwable cause) { + super(message, cause); + } Review Comment: The other two constructors without message arguments should be removed to require some description. ########## nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/action/HeadlessFlowActionReporter.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.action; + +import java.util.Collection; + +/** + * A headless {@link FlowActionReporter} that does not report any actions. + */ +public class HeadlessFlowActionReporter implements FlowActionReporter { Review Comment: Move to `nifi-headless-server` ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/action/HeadlessActionConverter.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.action; + +/** + * Headless implementation of the {@link ActionConverter} interface that converts to an {@link EmptyFlowAction}. + */ +public class HeadlessActionConverter implements ActionConverter { + + @Override + public FlowAction convert(Action action) { + return new EmptyFlowAction(); + } Review Comment: Formatting: ```suggestion } ``` ########## nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/action/HeadlessActionConverter.java: ########## @@ -0,0 +1,28 @@ +/* + * 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.action; + +/** + * Headless implementation of the {@link ActionConverter} interface that converts to an {@link EmptyFlowAction}. + */ +public class HeadlessActionConverter implements ActionConverter { + + @Override + public FlowAction convert(Action action) { Review Comment: ```suggestion public FlowAction convert(final Action action) { ``` ########## nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/resources/META-INF/services/org.apache.nifi.action.FlowActionReporter: ########## @@ -0,0 +1,15 @@ +# 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. +org.apache.nifi.action.HeadlessFlowActionReporter Review Comment: This service definition does not seem necessary, can it be removed? -- 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]
