I think the question from Phillip about how to make PR review easier is a great one. For any PR, providing a sample flow which exercises the change is a big time saver already. Demonstrating that unit tests are added/updated, asserting that license considerations have been addressed, and that checkstyle is good are all indicators that the contributor has put comprehensive thought and care into the submission.
For interactions with external services (extensions, authentication & authorization, etc.), providing a Docker image with the external services configured or another one-click (or as close to it) way to deploy those dependencies drastically decreases the time and energy a reviewer must expend. I think there is a lot of “hidden cost” that we as committers have not made visible and so for any one PR the contributor is justified in thinking “why aren’t they looking at my code, it’s 20 lines?”. For a committer to review that though, requires pausing any active development work they are doing, checking out a new branch, building the whole project (~45 minutes with tests & checkstyle on decent hardware), deploying and configuring any external dependencies, running the application and exercising the change, checking for edge cases, etc. All of that is independent from the actual code review process for logic, vulnerabilities, performance, adherence to best practices & design principles, edge cases, and supportability & maintainability by the community at large. There are a number of times where the preferable path is for a contributor to make the code available publicly in their own GitHub repository. This sidesteps the community review aspect, but also means that they are not reliant on the community at large for release and development timelines, nor subject to a response of “you are the only person who understands how [external component X] works, so we as a community cannot take on the support and maintenance responsibilities of this contribution." Andy LoPresto [email protected] [email protected] He/Him PGP Fingerprint: 70EC B3E5 98A6 5A3F D3C4 BACE 3C6E F65B 2F7D EF69 > On Oct 8, 2020, at 9:52 AM, Nissim Shiman <[email protected]> wrote: > > Hello Joe, Phillip and Pierre, > I am really happy we are having this discussion. > I (and very likely other contributers) was under the impression that only > reviews by committers/PMC members (i.e. more senior nifi dev community > members) had any real weight, so that is why I have not been reviewing > standing PRs. > > > So this discussion is bringing out out that anyone can review/test open PRs > and once any issues in the PRs have been smoothed out that it shouldn't be > much of an issue to have it committed at that point. > > Actually, it looks like a fellow contributer, Mark Bean, has already noticed > this conversation and has already reviewed my PR and has made some useful > comments. So we are already seeing results. (and thank you Mark!) > > I plan to start reviewing/testing PRs. > Thank You, > Nissim Shiman > On Thursday, October 8, 2020, 12:17:13 PM EDT, Joe Witt > <[email protected]> wrote: > > Hello > > Anyone can do a review of the code and build and feasibility of a change. > It only requires commit privileges to actually merge something. > > Doing the hard work of reviewing/testing the PRs is a *great* way to earn > merit. > > Much of the PRs that tend to hang around are from people contributing a > specific thing and those are some of the most challenging. We as a > community then take on support of those things for function, License and > Notice adherence, etc.. On the other hand, these folks are also the ones > that can often start contributing elsewhere and building merit and earning > commit and even PMC status. So we have to invest one way or another. But > for sure this is not easy to do well as time has shown. > > Nissim, in your case the contribution is probably cool/good/useful. What > could help you get more attention on it is helping with bug fixes or > reviewing other peoples contributions/etc.. Not saying anyone is checking > whether you do that or not but if they see more of your effort it *does* > help improve likelihood of getting merge attention. Not required...just > sharing another perspective. > > Regardless thanks for the contribution and hopefully someone is able to > review soon. > > Thanks > > On Thu, Oct 8, 2020 at 9:00 AM Phillip Grenier <[email protected]> wrote: > >> Pierre, >> >> I think this discussion brings up a valid conversation point. At some point >> a PMC member needs to approve the merge request, so from a contributors >> level what can we do to make that merge both easier and/or more likely to >> happen. That and how the community can help filter down the ever growing >> backlog of PRs. >> >> Thanks and look forward to helping, >> >> Phillip >> >> On Wed, Oct 7, 2020 at 2:04 PM Pierre Villard <[email protected] >>> >> wrote: >> >>> Nissim, >>> >>> Thanks a lot for your contribution but there are 277 pull requests opened >>> at this time. This is a community effort, and if you expect people to >>> review your PRs, you'd have to also try reviewing PRs opened by others in >>> the community. Otherwise this will need to wait for someone with some >>> bandwidth to focus on this. >>> >>> Thanks, >>> Pierre >>> >>> Le mer. 7 oct. 2020 à 19:59, Nissim Shiman <[email protected]> a >>> écrit : >>> >>>> Second attempt... >>>> >>>> >>>> Hello NiFi team, >>>> >>>> Could someone respond to this email. >>>> >>>> Thanks, >>>> Nissim Shiman >>>> On Monday, October 5, 2020, 11:01:57 AM EDT, Nissim Shiman >>>> <[email protected]> wrote: >>>> >>>> Hello NiFi team, >>>> Could someone review the pull request for NIFI-7738 ( >>>> https://issues.apache.org/jira/browse/NIFI-7738), that was put in 5 >> days >>>> ago? >>>> (Provenance Search Events - add feature to allow inverse query) >>>> >>>> One case this will come in handy is if a particular flowfile has an >> issue >>>> and goes in circles through processors and ends up emitting a lot of >>>> provenance making it hard to follow other flowfiles. >>>> >>>> This feature will allow users to query for all provenance except for >> the >>>> uuid of the problematic flowfile. >>>> >>>> >>>> >>>> This can also be used for any indexed field or attribute as well. >>>> This has been compiled and tested on java 8 and java11 for all 4 >>>> provenance repository implementations for both indexed fields and >>> indexed >>>> attributes >>>> The only "real" code changes were made to LuceneUtil.java (to handle >>>> WriteAhead, EncryptedWriteAhead and Persistent Provenance Repostories) >>> and >>>> VolatileProvenanceRepository.java >>>> The rest are just involved in propagating the ability to query the >> "NOT" >>>> option for a given user entered provenance search value from the gui to >>> the >>>> backend query. >>>> >>>> Thank You, >>>> Nissim Shiman >>>
