MINIFICPP-2462 - Divide libminifi #1902

adamdebreceni · 2024-11-27T11:45:22Z

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit?

For code changes:

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.

lordgamez

A few additional comments we talked about in person that should be done afterwards in different PRs and should create separate Jira tickets for:

we should add Impl suffix to core/* files in future
Move core/include/minifi-cpp directory minifi-api/include/minifi-cpp-api and later have a separate minifi-c-api directory for C API
remove ThreadedRepository.h from minifi api and replace its usage with core::TraceableResource

lordgamez · 2024-11-27T16:06:43Z

core/include/minifi-cpp/core/FlowFile.h

+// FlowFile Attribute
+struct SpecialFlowAttribute {
+  // The flowfile's path indicates the relative directory to which a FlowFile belongs and does not contain the filename
+  MINIFIAPI static constexpr std::string_view PATH = "path";
+  // The flowfile's absolute path indicates the absolute directory to which a FlowFile belongs and does not contain the filename
+  MINIFIAPI static constexpr std::string_view ABSOLUTE_PATH = "absolute.path";
+  // The filename of the FlowFile. The filename should not contain any directory structure.
+  MINIFIAPI static constexpr std::string_view FILENAME = "filename";
+  // A unique UUID assigned to this FlowFile.
+  MINIFIAPI static constexpr std::string_view UUID = "uuid";
+  // A numeric value indicating the FlowFile priority
+  MINIFIAPI static constexpr std::string_view priority = "priority";
+  // The MIME Type of this FlowFile
+  MINIFIAPI static constexpr std::string_view MIME_TYPE = "mime.type";
+  // Specifies the reason that a FlowFile is being discarded
+  MINIFIAPI static constexpr std::string_view DISCARD_REASON = "discard.reason";
+  // Indicates an identifier other than the FlowFile's UUID that is known to refer to this FlowFile.
+  MINIFIAPI static constexpr std::string_view ALTERNATE_IDENTIFIER = "alternate.identifier";
+  // Flow identifier
+  MINIFIAPI static constexpr std::string_view FLOW_ID = "flow.id";


I think these comments can be removed

lordgamez · 2024-11-27T16:07:14Z

core/include/minifi-cpp/core/ObjectFactory.h

+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {


These namespaces can be merged

lordgamez · 2024-11-27T16:08:01Z

core/include/minifi-cpp/core/ObjectFactory.h

+/**
+ * Factory that is used as an interface for
+ * creating processors from shared objects.
+ */


Is this comment valid?

lordgamez · 2024-11-27T16:10:02Z

core/include/minifi-cpp/core/ProcessContext.h

+#include <algorithm>
+#include <atomic>
+#include <concepts>
+#include <map>
+#include <memory>
+#include <mutex>
+#include <optional>
+#include <queue>
+#include <string>
+#include <unordered_map>
+#include <vector>


Some includes like algorithm, athomic, concepts, map, mutex, queue, unordered_map can be removed

lordgamez · 2024-11-27T16:10:53Z

core/include/minifi-cpp/core/ProcessContextBuilder.h

+#include <string>
+#include <vector>
+#include <queue>
+#include <map>
+#include <mutex>
+#include <atomic>
+#include <algorithm>
+#include <memory>


Vector, queue, atomic, mutex, map can be removed here (maybe Property.h and VariableRegistry.h as well)

lordgamez · 2024-11-28T11:21:28Z

utils/include/utils/Literals.h

+ */
+#pragma once
+
+#include "minifi-cpp/utils/Literals.h"


Same as before

lordgamez · 2024-11-28T11:21:58Z

utils/include/utils/MinifiConcurrentQueue.h

+#ifndef LIBMINIFI_INCLUDE_UTILS_MINIFICONCURRENTQUEUE_H_
+#define LIBMINIFI_INCLUDE_UTILS_MINIFICONCURRENTQUEUE_H_


This can be changed to #pragma once

lordgamez · 2024-11-28T13:04:59Z

utils/include/utils/MinifiConcurrentQueue.h

+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {


We can merge these namespaces

lordgamez · 2024-11-28T13:05:20Z

utils/include/utils/gsl.h

+
+#pragma once
+
+#include "minifi-cpp/utils/gsl.h"


Same as before

lordgamez · 2024-11-28T13:06:01Z

utils/src/core/Core.cpp

 // Set UUID
-void CoreComponent::setUUID(const utils::Identifier& uuid) {
+void CoreComponentImpl::setUUID(const utils::Identifier& uuid) {
  uuid_ = uuid;
 }

 // Get UUID
-utils::Identifier CoreComponent::getUUID() const {
+utils::Identifier CoreComponentImpl::getUUID() const {
  return uuid_;
 }

 // Set Processor Name
-void CoreComponent::setName(std::string name) {
+void CoreComponentImpl::setName(std::string name) {
  name_ = std::move(name);
 }
 // Get Process Name


These useless comments can be removed

martinzink

I've started to implement my new PR-s on top of this.
Dont want to further increase the size of the PR with nitpicks (since you touched almost the whole codebase I think we can skip the clang-tidy fixes aswell)
Great work LGTM 👍 please rebase and fix the rebase issues

adamdebreceni mentioned this pull request Nov 27, 2024

MINIFICPP-2462 - Divide libminifi #1870

Closed

8 tasks

lordgamez reviewed Nov 28, 2024

View reviewed changes

adamdebreceni and others added 3 commits December 2, 2024 12:46

Separate out minifi-api/extension-utils/utils from libminifi

3caf117

removed duplicate utils

3a3585e

Rebase

53c2983

adamdebreceni force-pushed the minifi-api branch from f5fecde to 53c2983 Compare December 2, 2024 11:49

adamdebreceni added 4 commits December 2, 2024 13:43

Remove more duplicates, rebase fix

ca5bf2b

Rebase fix

5e526de

Rebase fix

c6d8f0c

Rename to minifi-api

2656f32

martinzink approved these changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-2462 - Divide libminifi #1902

MINIFICPP-2462 - Divide libminifi #1902

adamdebreceni commented Nov 27, 2024

lordgamez left a comment

lordgamez Nov 27, 2024

lordgamez Nov 27, 2024

lordgamez Nov 27, 2024

lordgamez Nov 27, 2024

lordgamez Nov 27, 2024

lordgamez Nov 28, 2024

lordgamez Nov 28, 2024

lordgamez Nov 28, 2024

lordgamez Nov 28, 2024

lordgamez Nov 28, 2024

martinzink left a comment •

edited

Loading

		#ifndef LIBMINIFI_INCLUDE_UTILS_MINIFICONCURRENTQUEUE_H_
		#define LIBMINIFI_INCLUDE_UTILS_MINIFICONCURRENTQUEUE_H_


		#pragma once

		#include "minifi-cpp/utils/gsl.h"

MINIFICPP-2462 - Divide libminifi #1902

Are you sure you want to change the base?

MINIFICPP-2462 - Divide libminifi #1902

Conversation

adamdebreceni commented Nov 27, 2024

For all changes:

For code changes:

For documentation related changes:

Note:

lordgamez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinzink left a comment • edited Loading

Choose a reason for hiding this comment

martinzink left a comment •

edited

Loading