NIFI-14837 - Performance improvement GitHub Registry Client

[pvillard31]

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

• 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 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 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[exceptionfactory]

Thanks for proposing this improvement @pvillard31, adding caching and limiting commits retrieved from GitHub are helpful improvements.

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.

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?

[pvillard31]

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.

[exceptionfactory]

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.

[pvillard31]

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?

[exceptionfactory]

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.

[pvillard31]

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?

[exceptionfactory]

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.

[pvillard31]

Sounds good, thanks @exceptionfactory, I have made the corresponding changes

[exceptionfactory]

Thanks for the updates @pvillard31, just one more question related to commit size retrieval.

[exceptionfactory]

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?

[pvillard31]

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.

[pvillard31]

Made a small change to use the client in order to only retrieve the first page of commits and only define a page size.

[exceptionfactory]

Thanks @pvillard31, the updated approach looks good. +1 merging