Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NCL-6205] make pnc-parallelism-threshold configurable #282

Merged
merged 4 commits into from
Dec 9, 2020

Conversation

michalovjan
Copy link
Collaborator

No description provided.

@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #282 (3c50953) into master (8a061fc) will increase coverage by 0.03%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #282      +/-   ##
============================================
+ Coverage     69.03%   69.07%   +0.03%     
- Complexity      621      624       +3     
============================================
  Files            38       38              
  Lines          3039     3049      +10     
  Branches        377      378       +1     
============================================
+ Hits           2098     2106       +8     
- Misses          768      769       +1     
- Partials        173      174       +1     
Impacted Files Coverage Δ Complexity Δ
...main/java/org/jboss/pnc/build/finder/cli/Main.java 24.39% <33.33%> (+0.06%) 15.00 <0.00> (ø)
...a/org/jboss/pnc/build/finder/core/BuildConfig.java 96.82% <100.00%> (+0.13%) 71.00 <3.00> (+3.00)
...a/org/jboss/pnc/build/finder/core/BuildFinder.java 77.79% <100.00%> (ø) 104.00 <0.00> (ø)
...rg/jboss/pnc/build/finder/core/ConfigDefaults.java 100.00% <100.00%> (ø) 1.00 <0.00> (ø)
...rg/jboss/pnc/build/finder/core/PncBuildFinder.java 63.33% <100.00%> (+0.24%) 27.00 <1.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a061fc...3c50953. Read the comment docs.

@dwalluck
Copy link
Collaborator

dwalluck commented Dec 1, 2020

There was already pnc-partition-size which was indirectly meant to control the number of threads. I see it is now used with:

configurationBuilder.pageSize(config.getPncPartitionSize());

which I am not sure is how it was intended and I'd prefer the name pnc-num-threads to match the koji option.

Should we change the name to pnc-page-size for the existing one then?

The original idea was if you had a collection of size 100 and a PNC partition size of 10, then you would have 100 / 10 = 10 concurrent requests/threads of 10 elements each.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -68,6 +68,7 @@
public static final URL PNC_URL = null;
public static final Boolean USE_BUILDS_FILE = Boolean.FALSE;
public static final Boolean USE_CHECKSUMS_FILE = Boolean.FALSE;
public static final Long PNC_PARALLELISM_THRESHOLD = 10L;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also sorted by name.

@michalovjan
Copy link
Collaborator Author

@dwalluck Sure, I am completely fine with the change to pnc-num-threads. OTOH, I wouldn't change the pnc-partition-size to pnc-page-size as this would break existing configurations which is not necessary and the current naming is apt.

@dwalluck
Copy link
Collaborator

dwalluck commented Dec 2, 2020

@dwalluck Sure, I am completely fine with the change to pnc-num-threads. OTOH, I wouldn't change the pnc-partition-size to pnc-page-size as this would break existing configurations which is not necessary and the current naming is apt.

To me it is confusing. I guess one possible thing is add @JsonAlias, but I guess it's too late now.

@michalovjan One last thing, can you add a line to core/src/test/java/org/jboss/pnc/build/finder/core/BuildConfigTest.java#verifyDefaults to verify the new config option? You can add to an existing or new method in cli/src/test/java/org/jboss/pnc/build/finder/cli/MainTest.java as well. These are the offline basic unit tests whereas the one you modified I believe is only online, so I prefer to have both kinds of tests.

@michalovjan
Copy link
Collaborator Author

@dwalluck Done :)

@michalovjan
Copy link
Collaborator Author

@dwalluck Is it good to merge?

@dwalluck dwalluck merged commit d7542bd into project-ncl:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants