-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
NIFI-11609 Support Request-Response pattern in MQTT5 Processors #7352
Conversation
processedRecords.getAndIncrement(); | ||
} | ||
} catch (SchemaNotFoundException | MalformedRecordException e) { | ||
throw new ProcessException("An error happened during creating components for serialization.", e); | ||
} | ||
} | ||
|
||
private String extractRecordValue(Record record, RecordPath recordPath) { | ||
final Optional<FieldValue> fv = recordPath.evaluate(record).getSelectedFields().findFirst(); | ||
if (fv.isPresent()) { |
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.
It is a bit misleading, because FieldValue is present even when there is no result with a given record path. This makes me wonder that is it a valid scenario when fv is empty.
PROP_RESPONSE_TOPIC, | ||
PROP_CORRELATION_DATA, | ||
PROP_CORRELATION_DATA_IS_RECORD_PATH, |
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.
@nandorsoma Thanks for adding this new feature in the MQTT processors!
The functionality looks good, however the configuration of the new properties is not really straightforward. E.g. when sending the response to a request, the response topic (contained in the original request) needs to be set in Topic
, not Response Topic
(Response Topic
is relevant for request messages, not responses). Also, the new properties may be confusing in the "normal" (non request/response) mode, so in most of the cases.
That's why I would suggest to document the request/response scenarios in the additional details page.
Also, introducing Messaging Type
(or Messaging Pattern
) property in PublishMQTT seems useful for me with the following allowable values:
- Standalone: default/current logic (no
Correlation Data
orResponse Topic
displayed) - Request: sending a request message and expecting a response to it (
Correlation Data
andResponse Topic
displayed) - Response: sending a response message to a received request (
Correlation Data
is displayed, butResponse Topic
is not)
The role/origin of Correlation Data
is a bit different in the last 2 cases. For requests, it is the message sender's responsibility to generate a unique correlation data. For responses, the message sender has to echo the received correlation data back. It can be mentioned in the property description or the additional details page.
Closing as the proposed solution with the extra fields is inconvenient. We need to find a generic way to specify raw value or record path in the same field. |
Summary
NIFI-11609
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
LICENSE
andNOTICE
filesDocumentation