[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r832234158
##
File path: extensions/http-curl/tests/C2ClearCoreComponentStateTest.cpp
##
@@ -65,7 +65,9 @@ class VerifyC2ClearCoreComponentState : public VerifyC2Base {
class ClearCoreComponentStateHandler: public HeartbeatHandler {
public:
- explicit ClearCoreComponentStateHandler(std::atomic_bool&
component_cleared_successfully) :
component_cleared_successfully_(component_cleared_successfully) {
+ explicit ClearCoreComponentStateHandler(std::atomic_bool&
component_cleared_successfully, std::shared_ptr
configuration)
+: HeartbeatHandler(std::move(configuration)),
+ component_cleared_successfully_(component_cleared_successfully) {
Review comment:
We are asserting on the variable while waiting until the timeout for its
value to return true on line 49.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r832094267
##
File path: extensions/http-curl/tests/C2VerifyHeartbeatAndStop.cpp
##
@@ -55,7 +40,7 @@ class VerifyC2Heartbeat : public VerifyC2Base {
void runAssertions() override {
using org::apache::nifi::minifi::utils::verifyLogLinePresenceInPollTime;
-
assert(verifyLogLinePresenceInPollTime(std::chrono::milliseconds(wait_time_),
+assert(verifyLogLinePresenceInPollTime(std::chrono::milliseconds(12),
Review comment:
Thanks for catching this, I only set this while testing something and I
forgot about it. Fixed in a064ba2a3bab0dbe4ded393a4d52dd5315c3f976
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r831923650
##
File path: libminifi/include/properties/ConfigurationReader.h
##
@@ -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.
+ */
+
+#pragma once
+
+#include
+#include
+
+namespace org::apache::nifi::minifi {
+
+class ConfigurationReader {
+ public:
+ virtual bool get(const std::string& key, std::string& value) const = 0;
+ virtual bool get(const std::string& key, const std::string& alternate_key,
std::string& value) const = 0;
+ virtual std::optional get(const std::string& key) const = 0;
+};
Review comment:
The reason was the signature of the `get` function of the `Configure`
class which expects `const std::string& key` as its parameter.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r831916481
##
File path: libminifi/include/properties/ConfigurationReader.h
##
@@ -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.
+ */
+
+#pragma once
+
+#include
+#include
+
+namespace org::apache::nifi::minifi {
+
+class ConfigurationReader {
+ public:
+ virtual bool get(const std::string& key, std::string& value) const = 0;
+ virtual bool get(const std::string& key, const std::string& alternate_key,
std::string& value) const = 0;
+ virtual std::optional get(const std::string& key) const = 0;
+};
Review comment:
I was referring to classes like the `AgentIdentificationProvider` or the
`StateMonitor` where those classes are also implemented as separate interfaces
to be later set in one of the agent information nodes. I think these adhere
more to the interface segregation principle and especially in case of the
FlowController where there are already way too many responsibilities they could
be later extracted to separate classes and composed instead of inheriting from
them. Also `AgentIdentificationProvider` is already one interface which is
extracted and implemented by the `Configure` class.
**BUT** I rechecked this case specifically and as you mentioned we only need
a single overload of the get functions and I'm not sure if this "read-only
configuration" interface will be reused in the future anywhere else, so after
these considerations I replaced it with a single std::function in
c2544df895ed1fac5ba6d5b50e27ecea4e073c3e
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r831916481
##
File path: libminifi/include/properties/ConfigurationReader.h
##
@@ -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.
+ */
+
+#pragma once
+
+#include
+#include
+
+namespace org::apache::nifi::minifi {
+
+class ConfigurationReader {
+ public:
+ virtual bool get(const std::string& key, std::string& value) const = 0;
+ virtual bool get(const std::string& key, const std::string& alternate_key,
std::string& value) const = 0;
+ virtual std::optional get(const std::string& key) const = 0;
+};
Review comment:
I was referring to classes like the `AgentIdentificationProvider` or the
`StateMonitor` where those classes are also implemented as separate interfaces
to be later set in one of the agent information nodes. I think these adhere
more to the interface segregation principle and especially in case of the
FlowController where there are already way too many responsibilities they could
be later extracted to separate classes and composed instead of inheriting from
them.
**BUT** I rechecked this case specifically and as you mentioned we only need
a single overload of the get functions and I'm not sure if this "read-only
configuration" interface will be reused in the future anywhere else, so after
these considerations I replaced it with a single std::function in
c2544df895ed1fac5ba6d5b50e27ecea4e073c3e
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r831141775
##
File path: libminifi/include/properties/ConfigurationReader.h
##
@@ -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.
+ */
+
+#pragma once
+
+#include
+#include
+
+namespace org::apache::nifi::minifi {
+
+class ConfigurationReader {
+ public:
+ virtual bool get(const std::string& key, std::string& value) const = 0;
+ virtual bool get(const std::string& key, const std::string& alternate_key,
std::string& value) const = 0;
+ virtual std::optional get(const std::string& key) const = 0;
+};
Review comment:
I decided to keep the interface for now. It's more consistent with the
other setters of the agent information nodes and in my opinion it's cleaner to
see what that member represents.
Also good catch, I missed the virtual destructor, added in last commit.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a change in pull request #1281: MINIFICPP-1769 Add propertyValue to agent manifest metadata
lordgamez commented on a change in pull request #1281:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1281#discussion_r831141775
##
File path: libminifi/include/properties/ConfigurationReader.h
##
@@ -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.
+ */
+
+#pragma once
+
+#include
+#include
+
+namespace org::apache::nifi::minifi {
+
+class ConfigurationReader {
+ public:
+ virtual bool get(const std::string& key, std::string& value) const = 0;
+ virtual bool get(const std::string& key, const std::string& alternate_key,
std::string& value) const = 0;
+ virtual std::optional get(const std::string& key) const = 0;
+};
Review comment:
I decided to keep the interface for now. It's more consistent with the
other setters of the agent information nodes and in my opinion it's cleaner to
see what that member represents.
--
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: issues-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
