Skip to content
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-13554 GetSlackReaction processor to fetch reactions for Slack message #9086

Closed

Conversation

krisztina-zsihovszki
Copy link
Contributor

Summary

NIFI-13554

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files


static final PropertyDescriptor CHANNEL_ID_JSON_PATH = new PropertyDescriptor.Builder()
.name("Channel ID JSON Path")
.description("The JSON Path which identifies the channel ID in the incoming JSON.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we require input to be in JSON format? It might be more flexible (and facilitate a future ConsumeSlackRecord processor for example) to use Record Path and a Record Reader?

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about the use of JSON Paths for several properties. If the input format is from other Processors, then making this configurable seems unnecessary.

Beyond that point, I'm more concerned about the scalability of this the check and wait strategy. The idea of the wait period property seems like it could easily lead to FlowFiles backed up in a queue, waiting for reactions that may never arrive. This presents some fundamental flow design concerns. I understand that retrieving reactions is separate from messages themselves, however, it seems that a more scalable approach would require either changes to ConsumeSlack, or a new ConsumeSlackReactions Processor. A feed of reactions allows for eventual merging with messages, as opposed to a Processor designed to loop and wait with a large number of polling requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants