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

Add proxy to avoid too much connections #353

Merged
merged 2 commits into from Feb 28, 2018

Conversation

Projects
None yet
2 participants
@Jimilian
Copy link
Contributor

commented Feb 15, 2018

In our environment we have several hundreds jobs, but only few unique dynamic trigger options (10 or 20). This PR has two main goals:

  • reduce memory consumption for establishing ssl connection / parsing dynamic trigger config list
  • speed dynamic config update time for duplicated entities
@rsandell
Copy link
Member

left a comment

Needs tests

ttl.put(url, System.currentTimeMillis());
cache.put(url, gerritProjects);

logger.info("Get dynamic projects directly for URL: {}", url);

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

Should probably be moved up to before the fetch is performed, or reworded.

* @return a list of GerritProjects if successful, or null if no change
* @throws ParseException when the fetched content couldn't be parsed
* @throws IOException for all other kinds of fetch errors
*/
public static List<GerritProject> fetch(String gerritTriggerConfigUrl, String serverName)
public static List<GerritProject> fetch(String gerritTriggerConfigUrl)

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

I get a strange twitch in my eye whenever a public method signature gets changed without a deprecated old signature left intact.
In this case I doubt anyone external uses it so it is most likely fine.
While you're in here could you add an @Restricted(NoExternalUse.class) on the class if that's possible in this version of core otherwise on each public method signature.

@@ -1716,33 +1716,27 @@ public void updateTriggerConfigURL() {
}
triggerInformationAction.setErrorMessage("");
try {
if (isAnyServer()) {
triggerInformationAction.setErrorMessage("Dynamic trigger configuration needs "

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

I can't remember why it needed this? And I get paranoid about it since I spent months tweaking the multi server configurations to not break.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Feb 20, 2018

Author Contributor

It was needed because very long time ago serverName in GerritDynamicUrlProcessor.fetch was really used.

@@ -115,7 +115,7 @@ private long calculateDynamicConfigRefreshInterval(@Nonnull GerritTrigger trigge
*
* @return the average value.
*/
private long calculateAverageDynamicConfigRefreshInterval() {
public long calculateAverageDynamicConfigRefreshInterval() {

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

@Restricted(NoExternalUse.class)

* It's used to speed up execution time during updating
* trigger jobs and reduce number of connections for the duplicated configs.
*/
final class CacheProxy {

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

This needs a better name to indicate that it's a proxy for fetching dynamic projects.

@Jimilian Jimilian force-pushed the Jimilian:proxy branch from 635cb9b to 5e98942 Feb 20, 2018

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2018

@rsandell tests were added, comments were addressed.


List<GerritProject> res1 = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache("someUrl");
List<GerritProject> res2 = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache("someUrl");

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

Should verify that GerritDynamicUrlProcessor.fetch( was called on the first call but not the second If I've understood the test correctly.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Feb 20, 2018

Author Contributor

Yes, this is done via verifyStatic() that is shortcut for verifyStatic(times(1)). I also found the name of this check a bit ... misleading.

setRefreshInternal(FORCE_REFRESH_INTERVAL);

List<GerritProject> res1 = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache("someUrl");
List<GerritProject> res2 = DynamicConfigurationCacheProxy.getInstance().fetchThroughCache("someUrl");

This comment has been minimized.

Copy link
@rsandell

rsandell Feb 20, 2018

Member

Should verify that GerritDynamicUrlProcessor.fetch( was called on both calls If I've understood the test correctly.

This comment has been minimized.

Copy link
@Jimilian

Jimilian Feb 20, 2018

Author Contributor

Yes, this is done via verifyStatic(times(2))

@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

@rsandell, please, advise if my understanding of Mockito API was wrong.

@rsandell

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

@Jimilian you are probably correct, that's why I didn't go any further :)
the reason why I haven't merged yet was because I was waiting for a response from @tekkamanendless just to make sure we didn't break his day :)

@rsandell rsandell merged commit 76f654c into jenkinsci:master Feb 28, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@rsandell thank you!

@Jimilian Jimilian deleted the Jimilian:proxy branch Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.