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-13468 Add standalone RecordPath function recordOf #9018

Closed
wants to merge 1 commit into from

Conversation

EndzeitBegins
Copy link
Contributor

@EndzeitBegins EndzeitBegins commented Jun 29, 2024

Summary

Add new standalone function recordOf to Record Path DSL.

See issue NIFI-13468 for details, especially regarding the difference to the existing mapOf.

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

@EndzeitBegins
Copy link
Contributor Author

I would love to hear your thoughts on this @pvillard31, as you were the one to introduce mapOf some months ago.

@joewitt
Copy link
Contributor

joewitt commented Jun 29, 2024

@EndzeitBegins It is effectively a non starter to merge new things into the 2.x/main line that are intentionally restrained to also go back into the 1.x line. We moved on for a lot of good reasons and the entire point is to leverage the language benefits of Java 21 and newer.

As-is given the above this PR should not proceed.

To be clear I'm not commenting on the idea itself - hopefully others can review/engage on that but the PR for this capability should target the 2.x line and benefits that come with that. I don't recommend backporting at all but that is not a hard rule.

@EndzeitBegins
Copy link
Contributor Author

Thank you for the fast feedback @joewitt.

It makes sense to avoid outdated language features targeting the 2.x branch. I'll adjust the PR accordingly.
If that's what we strive for, which seems desirable, we might need to watch out for that more on PR reviews though.

However I'm wondering, what are the reasons why you're against backports for the 1.x line while 2.x isn't stable yet?

@joewitt
Copy link
Contributor

joewitt commented Jun 29, 2024

  1. use latest language features/better code - thanks.
  2. Watch on PRs. - I assure you there are several of us that are. But there are a lot of contributors and it is massive codebase. We miss stuff.
  3. I am not against it. I simply have been for quite some time not recommending it. As long as people keep adding features and improvements to 1.x more than they focus on getting 2.0 done then it gets harder and harder to get 2.x done. What you focus on and wish to happen will happen. What you wait for....may not. Bottom line I am in the hearts and minds business. I recommend and encourage folks focus on the go forward solution. The 1.x line is great but it is already experiencing atrophy. I strongly believe our best way to help/support/grow the community is to look forward. This is just my view - you'll get an impressive amount of opinions on such a topic.

@EndzeitBegins
Copy link
Contributor Author

Thank you for addressing my questions @joewitt. I always appreciate getting to know the perspective and especially reasoning of others on topics.

I adjusted the PR accordingly. It mostly affected the tests though.

I would appreciate a review (not necessarily by you, of course).

@EndzeitBegins EndzeitBegins marked this pull request as draft September 5, 2024 04:16
@EndzeitBegins EndzeitBegins marked this pull request as ready for review September 5, 2024 19:57
@EndzeitBegins
Copy link
Contributor Author

Rebased onto latest commit on main and refactored tests based on changes introduced in NIFI-12852 / #9198 affecting the same file.

I'd appreciate a review.

Copy link
Contributor

@pvillard31 pvillard31 left a comment

Choose a reason for hiding this comment

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

Hey @EndzeitBegins - I reviewed the changes and it looks good to me. Waiting for the checks to complete and, if all green, I'll merge.

@EndzeitBegins
Copy link
Contributor Author

Thank you for the review and taking on the merge process @pvillard31. 🙂

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