briansolo1985 commented on code in PR #6438:
URL: https://github.com/apache/nifi/pull/6438#discussion_r979887567
##########
c2/c2-client-bundle/c2-client-service/src/main/java/org/apache/nifi/c2/client/service/C2HeartbeatFactory.java:
##########
@@ -106,6 +106,17 @@ private AgentInfo getAgentInfo(AgentRepositories repos,
RuntimeManifest manifest
return agentInfo;
}
+ private Set<SupportedOperation> getSupportedOperations(RuntimeManifest
manifest) {
+ Set<SupportedOperation> supportedOperations;
+ // supported operations has value only in case of minifi, therefore we
return empty collection if
Review Comment:
Not sure if we should put minifi specific code here. I think if we make
supportedOperations part of the C2 protocol, we should make it generic. In
other words. we could extend the RuntimeManifest with the supportedOperations
field and omit AgentManifest class. What do you think?
##########
c2/c2-client-bundle/c2-client-service/src/main/java/org/apache/nifi/c2/client/service/ManifestHashProvider.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.c2.client.service;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.nifi.c2.protocol.api.SupportedOperation;
+import org.apache.nifi.c2.protocol.component.api.Bundle;
+
+public class ManifestHashProvider {
+ private volatile String currentBundles = null;
+ private volatile Set<SupportedOperation> currentSupportedOperations =
Collections.emptySet();
+ private volatile Pair<Integer, String> currentHashCodeManifestHashMapping
= Pair.of(-1, null);
Review Comment:
I know Pair represents a mapping relationship between hash and manifest
hash, but for me it makes the code less comprehensible. We could define the
content of the pair as separate class members.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/c2/C2NifiClientService.java:
##########
@@ -90,15 +96,18 @@ public C2NifiClientService(final NiFiProperties
niFiProperties, final FlowContro
this.heartbeatPeriod = clientConfig.getHeartbeatPeriod();
this.flowController = flowController;
C2HttpClient client = new C2HttpClient(clientConfig, new
C2JacksonSerializer());
- C2HeartbeatFactory heartbeatFactory = new
C2HeartbeatFactory(clientConfig, flowIdHolder);
+ C2HeartbeatFactory heartbeatFactory = new
C2HeartbeatFactory(clientConfig, flowIdHolder, new ManifestHashProvider());
+ OperandPropertiesProvider emptyOperandPropertiesProvider = new
EmptyOperandPropertiesProvider();
Review Comment:
A question: do we need to inject operandPropertiesProvider from here to keep
the C2 layer decoupled from Minifi?
My feeling is that this class start to get a bit crowded, and as a glue
between Minifi and C2 I expect it to have even more logic in the future. I
think we should think about how we could extract logic into separate classes
(not strictly in the PR)
##########
c2/c2-client-bundle/c2-client-service/src/main/java/org/apache/nifi/c2/client/service/ManifestHashProvider.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.c2.client.service;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.nifi.c2.protocol.api.SupportedOperation;
+import org.apache.nifi.c2.protocol.component.api.Bundle;
+
+public class ManifestHashProvider {
+ private volatile String currentBundles = null;
Review Comment:
Will this class accessed by multiple threads, why do we need to define these
variables volatile?
If we want to avoid race conditions, I think we will need to apply locking
or define calculateManifestHash method synchronized.
--
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]
