You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "pvillard31 (via GitHub)" <gi...@apache.org> on 2025/08/09 16:50:52 UTC
[PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
pvillard31 opened a new pull request, #10186:
URL: https://github.com/apache/nifi/pull/10186
# Summary
NIFI-14837 - Performance improvement GitHub Registry Client
I'm using the GitHub Registry Client in my NiFi instance. I have about 50 process groups that are versioned. Every process group matches a versioned flow that may have tens of commits.
When I want to change version, the current implementation will list all commits, and for each commit, will make an API call to GiHub in order to retrieve some specific informations (commit message, commit date, etc).
This is extremely ineffective and changing the version a flow ends up taking a very long time. For some cases with many commits, I cannot change version because the call in the NiFi UI would time out before the backend has sent back the full list of commits with all of the information.
This becomes very not friendly and barely usable. This will also impact the API rate limits a lot.
This change is to introduce multiple improvements that are making all of this MUCH better.
- The GitHub client being used is initialized with an optional OkHttp client cache (see https://hub4j.github.io/github-api/)
> This library comes with a pluggable connector to use different HTTP client implementations through HttpConnector. In particular, this means you can use [OkHttp](https://square.github.io/okhttp/), so we can make use of its HTTP response cache. Making a conditional request against the GitHub API and receiving a 304 response [does not count against the rate limit](https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#conditional-requests).
- Adding a LRU cache to the client with a fixed size of 1000 commits maximum in order to keep an internal cache of commit SHA to commit details.
- Expose a property to limit the number of commits retrieved. The client does not ensure a chronological order but guarantees a topological ordering. So it should be chronological except in some specific edge cases like rebase, merge commits, cherry-pick, commits with manual dates, etc. However, this is very unlikely to happen with a normal usage of the client. Regardless the default is to retrieve all commits like it is right now.
- Adding a Rate Abuse Limit Handler to log an error when abusing the API limits.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### 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](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #10186:
URL: https://github.com/apache/nifi/pull/10186#discussion_r2276814000
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -276,8 +299,13 @@ public List<GitCommit> getCommits(final String path, final String branch) throws
.toList();
final List<GitCommit> commits = new ArrayList<>();
+ int i = 0;
for (final GHCommit ghCommit : ghCommits) {
+ if (i >= MAX_COMMITS_TO_RETRIEVE) {
+ break;
+ }
Review Comment:
Limiting this after retrieval does not seem like the best approach. The Page Size appears to limit the current number to 50. Would it be sufficient to switch that to 10?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3188315349
> Just to make sure we're on the same page:
>
> * have the option to limit the number of commits retrieved and expose this as a property
>
> * have an in-memory cache of the commits base on the commit hash with a hard coded value
>
>
> And remove the okhttp client cache that is file based.
>
> Right?
Slight adjustment to the first bullet, I recommend a hard limit on the number of commits retrieved, as I don't think it needs to be a configurable property.
Yes, on the in-memory cache with hard-coded limit, and yes on the removing the OkHttp client cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3188323432
> Slight adjustment to the first bullet, I recommend a hard limit on the number of commits retrieved, as I don't think it needs to be a configurable property.
That would prevent someone from being able to see the full list of versions for a given flow. While this is probably not necessary in 99% of the scenarios, hard coding the value would definitely prevent someone from seeing the full history. Are we OK with this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3186365729
Thanks for the reply @pvillard31.
If the most valuable improvement comes from limiting the number of commits returned, I would be in a favor of introducing a hard-coded default limit. If that proves to be too restrictive for some reason, revisiting this an introducing a configurable property could be a follow on effort.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3188370826
> > Slight adjustment to the first bullet, I recommend a hard limit on the number of commits retrieved, as I don't think it needs to be a configurable property.
>
> That would prevent someone from being able to see the full list of versions for a given flow. While this is probably not necessary in 99% of the scenarios, hard coding the value would definitely prevent someone from seeing the full history. Are we OK with this?
Yes, that's where I think at some point, the number of versions would be effectively unusable. So a large, but limited, number seems like a good approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3179595270
Thanks @exceptionfactory
> I am concerned about adding the Cache Directory property, because it introduces potential compatibility concerns on future upgrades. Although the cache format may not change very often, it introduces an additional layer of compatibility concern down the road. It seems better to restrict caching to something memory-based.
I did think about adding a custom in-memory cache via an OkHttp interceptor but I'm concerned about the size in case the flows that we retrieve are particularly huge. Unless I limit the interceptor to very specific endpoints being called like list commits, get commit details. But it felt overcomplicated. To be honest the most significant improvements are the commits caching and limiting the number of commits being retrieved so I'm also OK dropping the OkHttp caching for now.
> As far as maximum commits, having a limit seems reasonable, but I'm also not sure if this needs to be a configurable property. Setting a reasonable maximum seems like a safety feature to avoid unnecessary consumption, and going beyond a certain limit seems unnecessary and potentially unusable in the context of reviewing changes. Perhaps setting an internal limit of 1000 is the best approach for now?
I did think about it and I don't have a strong opinion. Something that I did consider is to add a property "Enable Caching" true/false that would make visible all of the new properties (including the one about how many commits are retrieved?) and expose this "magic number" that is currently set to 1000. No strong opinion tbh.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on code in PR #10186:
URL: https://github.com/apache/nifi/pull/10186#discussion_r2276882191
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -276,8 +299,13 @@ public List<GitCommit> getCommits(final String path, final String branch) throws
.toList();
final List<GitCommit> commits = new ArrayList<>();
+ int i = 0;
for (final GHCommit ghCommit : ghCommits) {
+ if (i >= MAX_COMMITS_TO_RETRIEVE) {
+ break;
+ }
Review Comment:
Made a small change to use the client in order to only retrieve the first page of commits and only define a page size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3188443250
Sounds good, thanks @exceptionfactory, I have made the corresponding changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory merged PR #10186:
URL: https://github.com/apache/nifi/pull/10186
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on code in PR #10186:
URL: https://github.com/apache/nifi/pull/10186#discussion_r2276827583
##########
nifi-extension-bundles/nifi-github-bundle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java:
##########
@@ -276,8 +299,13 @@ public List<GitCommit> getCommits(final String path, final String branch) throws
.toList();
final List<GitCommit> commits = new ArrayList<>();
+ int i = 0;
for (final GHCommit ghCommit : ghCommits) {
+ if (i >= MAX_COMMITS_TO_RETRIEVE) {
+ break;
+ }
Review Comment:
The page size does not limit how many commits we retrieve, only how many commits we have per page but the listing would go over all of the pages regardless. This is something in the implementation of the client itself. The only way to limit the amount of commits retrieved during listing would be by providing an "afterDate" value in the query builder.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-14837 - Performance improvement GitHub Registry Client [nifi]
Posted by "pvillard31 (via GitHub)" <gi...@apache.org>.
pvillard31 commented on PR #10186:
URL: https://github.com/apache/nifi/pull/10186#issuecomment-3187364380
Just to make sure we're on the same page:
- have the option to limit the number of commits retrieved and expose this as a property
- have an in-memory cache of the commits base on the commit hash with a hard coded value
And remove the okhttp client cache that is file based.
Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org