-
Notifications
You must be signed in to change notification settings - Fork 99
MINIFICPP-2650 - Processor c api #1987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b3fc327 to
23cad31
Compare
b4a4728 to
51d8a57
Compare
6932d42 to
75cdd49
Compare
fffbcd6 to
d86c0ab
Compare
f35808b to
9807e09
Compare
b860117 to
0c0310a
Compare
054fd16 to
a57636f
Compare
bb60828 to
4de141c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a comprehensive C API framework for MiniFi C++ that enables plugins to be written in C while leveraging the existing C++ infrastructure. The implementation adds a processor C API layer that allows C-based extensions to integrate seamlessly with the MiniFi processing pipeline.
Key changes include:
- New C API interface with MiniFi-specific types and callback structures
- C API framework implementation that bridges C extensions to C++ core functionality
- Refactored include paths and modularized architecture to support multiple API layers
- Enhanced extension loading system that can verify and load both C++ and C API extensions
Reviewed Changes
Copilot reviewed 102 out of 143 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| minifi-api/include/minifi-c/minifi-c.h | Complete C API interface definition with processor callbacks and data structures |
| libminifi/src/minifi-c.cpp | C API implementation that bridges C calls to C++ MiniFi functionality |
| core-framework/c-api-framework/ | New framework providing C++ wrappers for C API components |
| libminifi/include/utils/CProcessor.h | C processor wrapper enabling C extensions to participate in C++ processing pipeline |
| core-framework/include/core/extension/Utils.h | Enhanced extension verification supporting both C++ and C API libraries |
Comments suppressed due to low confidence (1)
libminifi/src/minifi-c.cpp:1
- This reinterpret_cast is unsafe and could lead to undefined behavior. The function should verify the type safety or use a safer casting mechanism.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
martinzink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #2029 is already merged could you please rebase and retarget this PR to main?
core-framework/c-api-framework/include/api/core/ProcessContext.h
Outdated
Show resolved
Hide resolved
core-framework/c-api-framework/include/api/core/ProcessorImpl.h
Outdated
Show resolved
Hide resolved
66b396a to
09ba849
Compare
09ba849 to
0d54774
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
early API comments
| uint32_t dynamic_properties_count; | ||
| const MinifiDynamicProperty* dynamic_properties_ptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're mixing description and runtime values here. In a processor description/manifest, dynamic properties are only represented as supported/unsupported, while their values are a runtime information. I would remove these members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need this for docs generation, as they are in a separate section
core-framework/c-api-framework/include/api/core/PublishedMetrics.h
Outdated
Show resolved
Hide resolved
core-framework/c-api-framework/include/api/core/logging/Logger.h
Outdated
Show resolved
Hide resolved
core-framework/c-api-framework/include/api/utils/ProcessorConfigUtils.h
Outdated
Show resolved
Hide resolved
e4a6e0b to
312f3b2
Compare
|
|
||
| virtual void setSupportedRelationships(std::span<const RelationshipDefinition> relationships) = 0; | ||
| virtual void setSupportedProperties(std::span<const PropertyReference> properties) = 0; | ||
| virtual void setSupportedProperties(std::span<const Property> properties) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we previously had a Property::getReference that worked incorrectly discarding types and allowed_values, since despite its name a Property is not easy to convert to PropertyReference, the former stores std::vector<std::string> while the latter uses std::span<const std::string_view> (this caused an issue in python processors as well)
| set_target_properties(${extension-name} PROPERTIES | ||
| RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin" | ||
| ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin" | ||
| WINDOWS_EXPORT_ALL_SYMBOLS TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the export macros in the C API extensions and not export all symbols. CMake's GenerateExportHeader can make it simpler. At least that's the vision, but maybe there are blockers preventing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only has utility in tests where we directly link the test to (in this case) libminifi-llamacpp.dll, since we are directly driving the processor in the test (not waiting for the extension to register the callbacks) we need access to all member function symbols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we no longer log IDs? because I don't see them on the interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the trimming + id append has been moved upstream into LoggerBase
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:
For documentation related changes:
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.