MDU changes to support parallel processing of config repo materials #5663

maheshp · 2019-01-04T10:55:41Z

Refer #5673 for more details.

ibnc

What's the reasoning behind reverting this?

ibnc · 2019-01-07T18:46:29Z

server/src/main/java/com/thoughtworks/go/server/materials/ConfigMaterialUpdater.java


 import java.io.File;

 /**
 * Updates configuration from repositories.
 */
-public class ConfigMaterialUpdateListener implements GoMessageListener<MaterialUpdateCompletedMessage> {
-    private static final Logger LOGGER = LoggerFactory.getLogger(ConfigMaterialUpdateListener.class);
+public class ConfigMaterialUpdater implements GoMessageListener<MaterialUpdateCompletedMessage> {


Why change the class name back to ConfigMaterialUpdater?

ConfigMaterialUpdateListener is more consistent with the naming conventions of the other material update listeners, e.g. MaterialUpdateListerner

It's also not consistent with the listener factory nor queue for this class

I agree on that part, would be better to have ConfigMaterialUpdateListener

ibnc · 2019-01-07T18:50:16Z

...main/java/com/thoughtworks/go/server/materials/ConfigMaterialPostUpdateListenersFactory.java

+import static java.util.stream.IntStream.range;
+
+@Component
+public class ConfigMaterialPostUpdateListenersFactory {


This should be named in a similar manner to the listener. Maybe this should be ConfigMaterialUpdateListersFactory or the listener's name should change

ibnc · 2019-01-07T18:50:29Z

server/src/main/java/com/thoughtworks/go/server/materials/ConfigMaterialPostUpdateQueue.java

+ * @understands messages about completed config material updates
+ */
+@Component
+public class ConfigMaterialPostUpdateQueue extends GoMessageQueue<MaterialUpdateCompletedMessage> {


The queue name should also be consistent with the other two corresponding classes

maheshp · 2019-01-08T05:30:16Z

@ibnc thanks for your inputs. I am trying to list few reasons for reverting #5550

I believe the behaviour and responsibilities of MaterialUpdateListener is different from ConfigMaterialUpdater(renamed to ConfigMaterialUpdateListener). While the MaterialUpdateListener is responsible to update a given material and add relevant modifications into the db(basically responsible for MDU), the ConfigMaterialUpdater is responsible to parse and merge the config repo post the MDU.
I think the MaterialUpdateListenerFactory should be used to create only MaterialUpdateListeners based on the material types rather than ConfigMaterialUpdaters which should be handled separately through a different SystemEnvironment which I have done through - config.material.post.update.threads
Also, I thought the MaterialUpdateService being notified with MaterialUpdateCompletedMessage upon completion of parsing and merging the config_repo by ConfigMaterialUpdater seemed a bit more simpler rather than the MaterialUpdateService notifying the ConfigMaterialUpdater to parse and merge a config repo.

If required I can get on a call and we can discuss this further.

ibnc · 2019-01-08T18:21:17Z

@ibnc thanks for your inputs. I am trying to list few reasons for reverting #5550

I believe the behaviour and responsibilities of MaterialUpdateListener is different from ConfigMaterialUpdater(renamed to ConfigMaterialUpdateListener). While the MaterialUpdateListener is responsible to update a given material and add relevant modifications into the db(basically responsible for MDU), the ConfigMaterialUpdater is responsible to parse and merge the config repo post the MDU.

I'm not sure I agree. My thinking is that GoRepoConfigDataSource is the one responsible for parsing and merging the configuration. ConfigMaterialUpdater or ConfigMaterialUpdateListerner is responsible for setup and checkout. This gets into the naming convention comments I left as well.

I think the MaterialUpdateListenerFactory should be used to create only MaterialUpdateListeners based on the material types rather than ConfigMaterialUpdaters which should be handled separately through a different SystemEnvironment which I have done through - config.material.post.update.threads

Agreed

Also, I thought the MaterialUpdateService being notified with MaterialUpdateCompletedMessage upon completion of parsing and merging the config_repo by ConfigMaterialUpdater seemed a bit more simpler rather than the MaterialUpdateService notifying the ConfigMaterialUpdater to parse and merge a config repo.

Makes sense.

If required I can get on a call and we can discuss this further.

I don't think we need to do that. I'd be curious to see what the performance boost of the PR would be. @tomzo Do you think you could test this out on the same instances you tested out #5550 on, and report back?

arvindsv · 2019-01-08T18:37:56Z

@ibnc

I'd be curious to see what the performance boost of the PR would be

I'd expect to see almost no performance boost from this PR. And, in my opinion, that's ok.

The biggest benefit I see is that we can increase/decrease the number of threads for the merging config-repo materials, independently of the number of MDU threads for non-config-repo materials. I believe this was part of your PR and from what I know, that behavior continues in this one. That's where we saw the most performance boost.

In this PR, the number of config material update listeners can be changed and are independent of the threads for non-config-repo materials. That's what I like about this one.

Overall, I think your PR set the standard for perf. This one makes it more configurable (adds that old property back) and changes the messaging (or listener framework) so that it doesn't come back to the material update service more than once for a single material.

Edit: If I understand correctly, the number of listener threads for MDU was a combination of normal material and config-repo material, after the earlier PR. The number of merging threads was introduced and made configurable. In this PR, the number of config-repo listener threads is independent of the number of non-config-repo listener threads.

maheshp · 2019-01-09T04:19:41Z

Edit: If I understand correctly, the number of listener threads for MDU was a combination of normal material and config-repo material, after the earlier PR. The number of merging threads was introduced and made configurable. In this PR, the number of config-repo listener threads is independent of the number of non-config-repo listener threads.

Yes that's right, listing the system properties used for configuring the threads around MDU and ConifgRepo parse/merge.

System Property	Default Value	Description
material.check.threads	10	No of threads used to update SCM Materials
dependency.material.check.threads	3	No of threads used to update Dependency Materials
material.config.check.threads	2	No of threads used to update ConfigRepo Materials
config.material.post.update.threads	2	No of threads used to parse & merge ConfigRepo post material update

maheshp · 2019-01-11T04:56:03Z

@tomzo your any inputs for this PR, when you get time.

tomzo · 2019-01-11T09:06:09Z

base/src/main/java/com/thoughtworks/go/util/SystemEnvironment.java

@@ -214,6 +214,8 @@

    public static GoIntSystemProperty DEPENDENCY_MATERIAL_UPDATE_LISTENERS = new GoIntSystemProperty("dependency.material.check.threads", 3);

+    public static GoIntSystemProperty CONFIG_MATERIAL_POST_UPDATE_LISTENERS = new GoIntSystemProperty("config.material.post.update.threads", 3);


@maheshp in the summary, you written this default was supposed to be 2. But here we have 3.

Oops, will change it to 2.

tomzo · 2019-01-11T10:48:58Z

I'd be curious to see what the performance boost of the PR would be. @tomzo Do you think you could test this out on the same instances you tested out #5550 on, and report back?

@ibnc I did build this PR and started server with all production repositories. But 19.1 removes DES support and we still have some repos with secrets encrypted with it. So I cannot really give you any number now, sorry.

ibnc · 2019-01-11T20:58:45Z

@tomzo no worries :)

arvindsv · 2019-01-11T22:15:37Z

Alright, you three. You have to decide on the names for these. :) It looks like everyone is fine with the change, but not happy with the names. Here's what I think it looks like:

Who wants to provide suggestions for names? The candidates for change are:

ConfigMaterialUpdater
ConfigMaterialPostUpdateListenersFactory (this is the thing that currently makes ConfigMaterialUpdater objects)
ConfigMaterialPostUpdateQueue

arvindsv · 2019-01-11T22:17:24Z

The suggestions I've heard are:

ConfigMaterialUpdater => ConfigMaterialUpdateListener
ConfigMaterialPostUpdateListenersFactory => ConfigMaterialUpdateListenerFactory
ConfigMaterialPostUpdateQueue => "The queue name should also be consistent with the other two corresponding classes"

ConfigMaterialUpdateQueue is already taken.

arvindsv · 2019-01-11T22:27:24Z

"You three" (above) are @maheshp, @ibnc and @tomzo. :)

ibnc · 2019-01-14T18:35:08Z

The suggestions I've heard are:

ConfigMaterialUpdater => ConfigMaterialUpdateListener

ConfigMaterialPostUpdateListenersFactory => ConfigMaterialUpdateListenerFactory

ConfigMaterialPostUpdateQueue => "The queue name should also be consistent with the other two corresponding classes"

ConfigMaterialUpdateQueue is already taken.

I'm in favor of going the route of ConfigMateterialUpdateListener and ConfigMaterialUpdateListenerFactory

I'm actually okay with the queue name not changing

This reverts commit 0a15ede.

…r' ditto for factory.

arvindsv · 2019-01-15T16:28:15Z

Thanks everyone. These changes were made by @maheshp. Merged.

maheshp mentioned this pull request Jan 7, 2019

Bring back priority threads to process ConfigRepo materials #5673

Closed

ibnc reviewed Jan 7, 2019

View reviewed changes

maheshp force-pushed the refactor_mdu branch from 26ec2e5 to 11615b5 Compare January 8, 2019 04:54

maheshp added this to the Release 19.1.0 milestone Jan 9, 2019

maheshp added this to In progress in 19.1.0 via automation Jan 9, 2019

maheshp force-pushed the refactor_mdu branch from 11615b5 to b329ab4 Compare January 11, 2019 04:50

maheshp changed the title ~~WIP - MDU changes to support parallel processing of config repo materials~~ MDU changes to support parallel processing of config repo materials Jan 11, 2019

tomzo reviewed Jan 11, 2019

View reviewed changes

maheshp force-pushed the refactor_mdu branch from b329ab4 to 4315757 Compare January 11, 2019 11:22

maheshp added 3 commits January 15, 2019 11:11

Revert "Multithreaded Config Repo update"

a8693c5

This reverts commit 0a15ede.

Add ability to configure multiple ConfigRepoUpdaters to process post MDU

6c88929

Renamed class 'ConfigMaterialUpdater' to 'ConfigMaterialUpdateListene…

20b0b9c

…r' ditto for factory.

maheshp force-pushed the refactor_mdu branch from 4315757 to 20b0b9c Compare January 15, 2019 05:41

arvindsv merged commit 20b0b9c into gocd:master Jan 15, 2019

maheshp deleted the refactor_mdu branch January 16, 2019 03:25

maheshp moved this from In progress to Done in 19.1.0 Jan 16, 2019

adityasood moved this from Done to QA Done in 19.1.0 Jan 18, 2019

adityasood mentioned this pull request Jan 18, 2019

Loading many Config Repo Pipelines takes a long time #5010

Closed

chadlwilson mentioned this pull request Jul 5, 2022

Trigger pipeline without configrepo update #10560

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDU changes to support parallel processing of config repo materials #5663

MDU changes to support parallel processing of config repo materials #5663

maheshp commented Jan 4, 2019 •

edited

ibnc left a comment

ibnc Jan 7, 2019 •

edited

tomzo Jan 11, 2019

ibnc Jan 7, 2019

ibnc Jan 7, 2019

maheshp commented Jan 8, 2019

ibnc commented Jan 8, 2019

arvindsv commented Jan 8, 2019 •

edited

maheshp commented Jan 9, 2019

maheshp commented Jan 11, 2019

tomzo Jan 11, 2019

maheshp Jan 11, 2019

tomzo commented Jan 11, 2019

ibnc commented Jan 11, 2019

arvindsv commented Jan 11, 2019 •

edited

arvindsv commented Jan 11, 2019 •

edited

arvindsv commented Jan 11, 2019 •

edited

ibnc commented Jan 14, 2019

arvindsv commented Jan 15, 2019

		@@ -214,6 +214,8 @@

		public static GoIntSystemProperty DEPENDENCY_MATERIAL_UPDATE_LISTENERS = new GoIntSystemProperty("dependency.material.check.threads", 3);

		public static GoIntSystemProperty CONFIG_MATERIAL_POST_UPDATE_LISTENERS = new GoIntSystemProperty("config.material.post.update.threads", 3);

MDU changes to support parallel processing of config repo materials #5663

MDU changes to support parallel processing of config repo materials #5663

Conversation

maheshp commented Jan 4, 2019 • edited

ibnc left a comment

Choose a reason for hiding this comment

ibnc Jan 7, 2019 • edited

Choose a reason for hiding this comment

tomzo Jan 11, 2019

Choose a reason for hiding this comment

ibnc Jan 7, 2019

Choose a reason for hiding this comment

ibnc Jan 7, 2019

Choose a reason for hiding this comment

maheshp commented Jan 8, 2019

ibnc commented Jan 8, 2019

arvindsv commented Jan 8, 2019 • edited

maheshp commented Jan 9, 2019

maheshp commented Jan 11, 2019

tomzo Jan 11, 2019

Choose a reason for hiding this comment

maheshp Jan 11, 2019

Choose a reason for hiding this comment

tomzo commented Jan 11, 2019

ibnc commented Jan 11, 2019

arvindsv commented Jan 11, 2019 • edited

arvindsv commented Jan 11, 2019 • edited

arvindsv commented Jan 11, 2019 • edited

ibnc commented Jan 14, 2019

arvindsv commented Jan 15, 2019

maheshp commented Jan 4, 2019 •

edited

ibnc Jan 7, 2019 •

edited

arvindsv commented Jan 8, 2019 •

edited

arvindsv commented Jan 11, 2019 •

edited

arvindsv commented Jan 11, 2019 •

edited

arvindsv commented Jan 11, 2019 •

edited