[
https://issues.apache.org/jira/browse/NIFI-16359?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jannik Rebmann updated NIFI-16359:
----------------------------------
Description:
h2. Summary
GitLabFlowRegistryClient against a self-hosted GitLab instance generates high
load on the
GitLab server as the number of versioned process groups grows. The load does
not come from
one expensive operation but from a per-process-group multiplier on the
version-check path,
compounded by a GitLab-specific full-history listing. This ticket proposes
removing that
multiplier at the shared base-class level so all Git-based registry clients
benefit.
h2. Environment / Observation
Self-hosted GitLab, many versioned process groups. Our infrastructure team
flagged a high
volume of GitLab REST API calls (repository/commits, repository/tree,
repository/files)
originating from NiFi. The volume scales with the number of versioned process
groups, even
when most of them are bound to the *same* flow at the *same* version.
h2. Root cause
The dominant driver is a per-process-group multiplier, not the cost of a single
listing:
1. The periodic "up to date" check and version listing run {*}per process
group{*}. In
AbstractGitFlowRegistryClient, getLatestVersion(...), getFlowVersions(...) and
the
latest-version comparison in getFlowContents(...) all call
repositoryClient.getCommits(filePath, branch). With N process groups bound to
the same
flow (identical filePath+branch), each cycle issues N identical
repository/commits
requests. There is no deduplication across process groups.
2. repositoryClient is a singleton: getRepositoryClient(context) builds the
client once
(guarded by clientInitialized) and reuses the volatile instance. A cache placed
on the
base class is therefore shared across every process group using the component -
i.e. the
multiplier can be removed with a shared, in-memory cache.
3. GitLab-specific amplification: GitLabRepositoryClient.getCommits(path,
branch) uses the
gitlab4j List overload that delegates to Pager.all(), which pages the *entire*
commit
history for the path (the client sets per_page=100). Each of the N calls above
is thus
itself several page requests.
4. Content resolution (repository/tree, repository/files) is likewise performed
per process
group when materializing a version.
h2. Relationship to NIFI-14837 / PR #10186
NIFI-14837 (fixed in 2.6.0, PR #10186) improved the GitHub client by (a)
limiting the commit
listing to the first page and (b) adding an in-memory SHA->commit-detail cache.
Two points:
* Those changes were applied only to GitHubRepositoryClient. GitLab has
neither, and
additionally pages the full history (point 3 above).
* PR #10186 reduced GitHub load from O(processGroups x commits) to
O(processGroups): it
removed the full-history paging and the per-commit N+1 detail fetch. It did NOT
remove the
per-process-group multiplier - GitHubRepositoryClient.getCommits(path, branch)
still issues
a live getRef + first-page listing on every call, with no (path, branch) cache
and no TTL.
This simply was not visible at a small number of process groups. So GitHub
scales linearly
with process-group count as well.
* The SHA->commit-detail cache is not the load-bearing part for GitLab.
gitlab4j's commit
listing returns fully-populated Commit objects (author, message, committed_date
inline),
so there is no per-commit detail call to eliminate. Porting that cache 1:1 to
GitLab would
add a dependency and complexity without reducing API calls.
h2. Proposed solution
1. Primary, general fix - cache the commit listing at the base class. Add a
short-TTL,
size-bounded in-memory cache keyed by (filePath, branch) in
AbstractGitFlowRegistryClient,
wrapping the getCommits calls in getLatestVersion / getFlowVersions /
getFlowContents.
This collapses the per-process-group calls to one remote call per (flow,
branch) per TTL
window and benefits all Git clients (GitHub, GitLab, Bitbucket, Azure DevOps).
TTL bounds
the staleness for detecting a *remote* new version; invalidate eagerly on local
writes
(createContent, deleteContent) and on client (re)initialization so locally
committed
versions are visible immediately. TTL and size hard-coded initially; can be
promoted to
properties in a follow-up.
2. GitLab-specific - bound the commit listing to the first page (a small
COMMIT_PAGE_SIZE)
instead of Pager.all(), to stop full-history paging and cap payload size,
mirroring the
listing limit from PR #10186.
3. Optional follow-up - immutable content cache. For the repository/tree and
repository/files load, add a cache keyed by (commitSha, path). Content at a
fixed SHA is
immutable, so this can be cached without a TTL.
h2. Scope question for reviewers
The primary fix lives in the shared base class and benefits all four Git
clients, so this
ticket's scope effectively broadens beyond GitLab. Should NIFI-16359 own the
base-class
caching (with GitLab as the motivating case), or should the base-class change
be split into a
dedicated framework ticket that per-client tickets reference? Input welcome
before
implementation.
h2. References
NIFI-14837, PR [https://github.com/apache/nifi/pull/10186]
was:
We are using GitLabFlowRegistryClient against a self-hosted GitLab instance.
Our infrastructure team flagged a high load on the GitLab server that is being
generated by the NiFi flow registry client. With a larger number of versioned
process groups, NiFi's periodic version checks and version listings translate
into a high volume of GitLab REST API calls (repository/commits,
repository/tree, repository/files).
Looking into it, NIFI-14837 (fixed in 2.6.0, PR #10186) already reduced GitHub
API load in GitHubFlowRegistryClient by
(a) adding an in-memory LRU cache of commit details keyed by SHA (max 1000), and
(b) limiting commit listing to the first page instead of paging the full
history.
These optimizations were applied only to GitHubRepositoryClient. The sibling
GitLabRepositoryClient (nifi-gitlab-bundle) still has no caching and, in
getCommits(path, branch), calls CommitsApi.getCommits(projectPath, branch,
resolvedPath), which pages the entire commit history for the path (default
100/page). This is the most likely driver of the load our GitLab team is seeing.
Proposal: apply the same two improvements to GitLabRepositoryClient - an
internal commit cache keyed by commit SHA, and a bounded commit listing (e.g.
first page / configurable or hard limit) - mirroring the approach in PR #10186.
Reference: NIFI-14837, PR [https://github.com/apache/nifi/pull/10186]
> Port GitHubFlowRegistryClient API-call optimizations (NIFI-14837) to
> GitLabFlowRegistryClient
> ---------------------------------------------------------------------------------------------
>
> Key: NIFI-16359
> URL: https://issues.apache.org/jira/browse/NIFI-16359
> Project: Apache NiFi
> Issue Type: Improvement
> Components: Flow Versioning
> Affects Versions: 2.12.0
> Reporter: Jannik Rebmann
> Priority: Major
>
> h2. Summary
> GitLabFlowRegistryClient against a self-hosted GitLab instance generates high
> load on the
> GitLab server as the number of versioned process groups grows. The load does
> not come from
> one expensive operation but from a per-process-group multiplier on the
> version-check path,
> compounded by a GitLab-specific full-history listing. This ticket proposes
> removing that
> multiplier at the shared base-class level so all Git-based registry clients
> benefit.
> h2. Environment / Observation
> Self-hosted GitLab, many versioned process groups. Our infrastructure team
> flagged a high
> volume of GitLab REST API calls (repository/commits, repository/tree,
> repository/files)
> originating from NiFi. The volume scales with the number of versioned process
> groups, even
> when most of them are bound to the *same* flow at the *same* version.
> h2. Root cause
> The dominant driver is a per-process-group multiplier, not the cost of a
> single listing:
> 1. The periodic "up to date" check and version listing run {*}per process
> group{*}. In
> AbstractGitFlowRegistryClient, getLatestVersion(...), getFlowVersions(...)
> and the
> latest-version comparison in getFlowContents(...) all call
> repositoryClient.getCommits(filePath, branch). With N process groups bound to
> the same
> flow (identical filePath+branch), each cycle issues N identical
> repository/commits
> requests. There is no deduplication across process groups.
> 2. repositoryClient is a singleton: getRepositoryClient(context) builds the
> client once
> (guarded by clientInitialized) and reuses the volatile instance. A cache
> placed on the
> base class is therefore shared across every process group using the component
> - i.e. the
> multiplier can be removed with a shared, in-memory cache.
> 3. GitLab-specific amplification: GitLabRepositoryClient.getCommits(path,
> branch) uses the
> gitlab4j List overload that delegates to Pager.all(), which pages the
> *entire* commit
> history for the path (the client sets per_page=100). Each of the N calls
> above is thus
> itself several page requests.
> 4. Content resolution (repository/tree, repository/files) is likewise
> performed per process
> group when materializing a version.
> h2. Relationship to NIFI-14837 / PR #10186
> NIFI-14837 (fixed in 2.6.0, PR #10186) improved the GitHub client by (a)
> limiting the commit
> listing to the first page and (b) adding an in-memory SHA->commit-detail
> cache. Two points:
> * Those changes were applied only to GitHubRepositoryClient. GitLab has
> neither, and
> additionally pages the full history (point 3 above).
> * PR #10186 reduced GitHub load from O(processGroups x commits) to
> O(processGroups): it
> removed the full-history paging and the per-commit N+1 detail fetch. It did
> NOT remove the
> per-process-group multiplier - GitHubRepositoryClient.getCommits(path,
> branch) still issues
> a live getRef + first-page listing on every call, with no (path, branch)
> cache and no TTL.
> This simply was not visible at a small number of process groups. So GitHub
> scales linearly
> with process-group count as well.
> * The SHA->commit-detail cache is not the load-bearing part for GitLab.
> gitlab4j's commit
> listing returns fully-populated Commit objects (author, message,
> committed_date inline),
> so there is no per-commit detail call to eliminate. Porting that cache 1:1 to
> GitLab would
> add a dependency and complexity without reducing API calls.
> h2. Proposed solution
> 1. Primary, general fix - cache the commit listing at the base class. Add a
> short-TTL,
> size-bounded in-memory cache keyed by (filePath, branch) in
> AbstractGitFlowRegistryClient,
> wrapping the getCommits calls in getLatestVersion / getFlowVersions /
> getFlowContents.
> This collapses the per-process-group calls to one remote call per (flow,
> branch) per TTL
> window and benefits all Git clients (GitHub, GitLab, Bitbucket, Azure
> DevOps). TTL bounds
> the staleness for detecting a *remote* new version; invalidate eagerly on
> local writes
> (createContent, deleteContent) and on client (re)initialization so locally
> committed
> versions are visible immediately. TTL and size hard-coded initially; can be
> promoted to
> properties in a follow-up.
> 2. GitLab-specific - bound the commit listing to the first page (a small
> COMMIT_PAGE_SIZE)
> instead of Pager.all(), to stop full-history paging and cap payload size,
> mirroring the
> listing limit from PR #10186.
> 3. Optional follow-up - immutable content cache. For the repository/tree and
> repository/files load, add a cache keyed by (commitSha, path). Content at a
> fixed SHA is
> immutable, so this can be cached without a TTL.
> h2. Scope question for reviewers
> The primary fix lives in the shared base class and benefits all four Git
> clients, so this
> ticket's scope effectively broadens beyond GitLab. Should NIFI-16359 own the
> base-class
> caching (with GitLab as the motivating case), or should the base-class change
> be split into a
> dedicated framework ticket that per-client tickets reference? Input welcome
> before
> implementation.
> h2. References
> NIFI-14837, PR [https://github.com/apache/nifi/pull/10186]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
