nifi icon indicating copy to clipboard operation
nifi copied to clipboard

NIFI-13468 Add standalone RecordPath function recordOf

Open EndzeitBegins opened this issue 1 year ago • 5 comments

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

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

Pull Request Formatting

  • [x] Pull Request based on current revision of the main branch
  • [x] 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

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

Licensing

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

Documentation

  • [x] Documentation formatting appears as expected in rendered files

EndzeitBegins avatar Jun 29 '24 15:06 EndzeitBegins

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

EndzeitBegins avatar Jun 29 '24 15:06 EndzeitBegins

@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.

joewitt avatar Jun 29 '24 15:06 joewitt

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?

EndzeitBegins avatar Jun 29 '24 15:06 EndzeitBegins

  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.

joewitt avatar Jun 29 '24 15:06 joewitt

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 avatar Jun 29 '24 17:06 EndzeitBegins

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.

EndzeitBegins avatar Sep 05 '24 20:09 EndzeitBegins

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

EndzeitBegins avatar Sep 06 '24 13:09 EndzeitBegins