DATAGO-75198: Dynamic config for configpush #185

rudraneel-chakraborty · 2024-06-18T13:35:21Z

What is the purpose of this change?

Asks of https://sol-jira.atlassian.net/browse/DATAGO-75198 (EMA side of things)

How was this change implemented?

Java code change

How was this change tested?

Is there anything the reviewers should focus on/be aware of?

...

…98-configpush-with-dynamic-config

rudraneel-chakraborty · 2024-06-20T00:07:27Z

service/application/pom.xml

+        <dependency>
+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-starter-validation</artifactId>
+        </dependency>


So that we can use Constraint validations such as @NotBlank @NotNull etc.

Example: https://github.com/SolaceProducts/event-management-agent/pull/185/files#diff-6bb24e9fb69000ce73f5944136797f3766f1c7a0146707771cc6466b3b757e0eR12-R20

rudraneel-chakraborty · 2024-06-20T00:09:07Z

...lication/src/main/java/com/solace/maas/ep/common/model/EventBrokerResourceConfiguration.java

 public class EventBrokerResourceConfiguration extends ResourceConfiguration {
+    @NotBlank
    private String id;
-    private String brokerType;
+    @NotBlank
    private String name;
-    private List<EventBrokerConnectionConfiguration> connections;
+    @NotEmpty
+    private List<@Valid EventBrokerConnectionConfiguration> connections;
+    @NotNull
    private ResourceConfigurationType resourceConfigurationType;


All the model classes are updated to use Constraint Validation. This is much cleaner and IMO provides clear understanding of the validation requirements

rudraneel-chakraborty · 2024-06-20T00:10:46Z

.../src/main/java/com/solace/maas/ep/event/management/agent/config/ValidationConfiguration.java

+public class ValidationConfiguration {
+
+    @Bean
+    public static LocalValidatorFactoryBean defaultValidator() {
+        LocalValidatorFactoryBean factoryBean = new LocalValidatorFactoryBean();
+        MessageInterpolatorFactory interpolatorFactory = new MessageInterpolatorFactory();
+        factoryBean.setMessageInterpolator(interpolatorFactory.getObject());
+        return factoryBean;
+    }
+
+    @Bean
+    public static MethodValidationPostProcessor methodValidationPostProcessor(Environment environment, Validator validator) {
+        MethodValidationPostProcessor processor = new MethodValidationPostProcessor();
+        boolean proxyTargetClass = environment.getProperty("spring.aop.proxy-target-class", Boolean.class, true);
+        processor.setProxyTargetClass(proxyTargetClass);
+        processor.setValidator(validator);
+        return processor;
+    }
+}


So that we can use Constraint validation in model classes

rudraneel-chakraborty · 2024-06-20T00:11:36Z

...ava/com/solace/maas/ep/event/management/agent/subscriber/SolacePersistentMessageHandler.java

-            log.error("Error while processing inbound message from queue for mopMessageSubclass: {}", mopMessageSubclass);
-            throw new IllegalArgumentException(e);
+            if (processor != null && message != null) {
+                log.error("Error while processing inbound message from queue for mopMessageSubclass: {}", mopMessageSubclass);
+                processor.onFailure(e, processor.castToMessageClass(message));
+            } else {
+                log.error("Unsupported message and/or processor encountered. Skipping processing", e);
+            }
+


Discussed with @mynecker. We MUST not throw exception from the message receiver thread, rather we should handle the exception. I already implemented the processor.onFailure for ConfigPushMessageProcessor. We SHOULD do the same thing for scan

processor.onFailure(e, processor.castToMessageClass(message)); exceptions that might be thrown within the onFailure(...) method should be handled, too. Logging the exception should be ok

rudraneel-chakraborty · 2024-06-20T00:11:57Z

...ava/com/solace/maas/ep/event/management/agent/subscriber/SolacePersistentMessageHandler.java

+    @Getter
    private PersistentMessageReceiver persistentMessageReceiver;


Use Lombok instead of writing the Getter

rudraneel-chakraborty · 2024-06-20T00:12:24Z

...maas/ep/event/management/agent/subscriber/messageProcessors/ScanCommandMessageProcessor.java

+    @Override
+    public void onFailure(Exception e, ScanCommandMessage message) {
+        log.debug("Requires implementation");
+    }


We must implement this for Scan.

That's coming up next? Can we potentially hit this after the PR is merged? Seems more like a warning at least.

@CameronRushton already implemented in another PR following this one

CameronRushton

Minor non-blocking comments

CameronRushton · 2024-06-20T17:39:31Z

...maas/ep/event/management/agent/service/SolaceResourceConfigurationToEventConverterTests.java

+    @Test
+    void testMapToMessagingServiceEvent() {


⛏️ Good candidate for soft assertions in this test

CameronRushton · 2024-06-20T17:41:10Z

...maas/ep/event/management/agent/service/SolaceResourceConfigurationToEventConverterTests.java

+            validateConstraintViolationException(invalidResource, Set.of("mapToMessagingServiceEvent.eventBrokerResource.name: must not be blank",
+                    "mapToMessagingServiceEvent.eventBrokerResource.id: must not be blank", "mapToMessagingServiceEvent.eventBrokerResource.connections: must" +
+                            " not be empty"));


💭 The path includes the method name too? Weird. There must be config that removes that part. Do we care though? Probably not.

CameronRushton · 2024-06-20T17:42:16Z

...maas/ep/event/management/agent/subscriber/messageProcessors/ScanCommandMessageProcessor.java

+    @Override
+    public void onFailure(Exception e, ScanCommandMessage message) {
+        log.debug("Requires implementation");
+    }


That's coming up next? Can we potentially hit this after the PR is merged? Seems more like a warning at least.

moodiRealist

Approved with a question comment.

moodiRealist · 2024-06-20T12:44:23Z

...lication/src/main/java/com/solace/maas/ep/common/model/EventBrokerResourceConfiguration.java

    private String id;
-    private String brokerType;


why was this removed?

This is not needed. We use ResourceConfigurationType enum instead

sonarqube-solacecloud · 2024-06-27T13:57:01Z

SonarQube Quality Gate

58.0% Coverage on New Code (is less than 80%)

See analysis details on SonarQube

rudraneel-chakraborty added 6 commits June 16, 2024 16:37

DATAGO-75198 wip

28c349e

Merge branch 'DATAGO-75198-dynamic-config-configpush' into DATAGO-751…

dda676c

…98-configpush-with-dynamic-config

DATAGO-75198 wip

7dee706

Merge branch 'DATAGO-75198-dynamic-config-configpush' into DATAGO-751…

a9aeb7d

…98-configpush-with-dynamic-config

DATAGO-75198 wip

b630765

DATAGO-75198 wip

9fedaff

rudraneel-chakraborty had a problem deploying to build_pr June 18, 2024 13:35 — with GitHub Actions Failure

DATAGO-75198 IT tests

43d0bf8

rudraneel-chakraborty temporarily deployed to build_pr June 20, 2024 00:06 — with GitHub Actions Inactive

rudraneel-chakraborty commented Jun 20, 2024

View reviewed changes

rudraneel-chakraborty marked this pull request as ready for review June 20, 2024 00:13

CameronRushton approved these changes Jun 20, 2024

View reviewed changes

moodiRealist approved these changes Jun 20, 2024

View reviewed changes

DATAGO-75198 bug fix

91b9cd6

rudraneel-chakraborty temporarily deployed to build_pr June 26, 2024 16:29 — with GitHub Actions Inactive

mynecker mentioned this pull request Jun 27, 2024

DATAGO-77217 EMA dynamic configuration for scan jobs #186

Merged

DATAGO-75198 pr comments

2ba49c3

rudraneel-chakraborty temporarily deployed to build_pr June 27, 2024 13:50 — with GitHub Actions Inactive

rudraneel-chakraborty merged commit 2ebd005 into main Jun 27, 2024
5 of 6 checks passed

rudraneel-chakraborty deleted the DATAGO-75198-configpush-with-dynamic-config branch June 27, 2024 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAGO-75198: Dynamic config for configpush #185

DATAGO-75198: Dynamic config for configpush #185

rudraneel-chakraborty commented Jun 18, 2024

rudraneel-chakraborty Jun 20, 2024

rudraneel-chakraborty Jun 20, 2024

rudraneel-chakraborty Jun 20, 2024

rudraneel-chakraborty Jun 20, 2024

rudraneel-chakraborty Jun 20, 2024

mynecker Jun 26, 2024

rudraneel-chakraborty Jun 27, 2024

rudraneel-chakraborty Jun 20, 2024

rudraneel-chakraborty Jun 20, 2024

CameronRushton Jun 20, 2024

mynecker Jun 26, 2024

CameronRushton left a comment

CameronRushton Jun 20, 2024

CameronRushton Jun 20, 2024

CameronRushton Jun 20, 2024

moodiRealist left a comment

moodiRealist Jun 20, 2024

rudraneel-chakraborty Jun 24, 2024

sonarqube-solacecloud bot commented Jun 27, 2024

		@Getter
		private PersistentMessageReceiver persistentMessageReceiver;

DATAGO-75198: Dynamic config for configpush #185

DATAGO-75198: Dynamic config for configpush #185

Conversation

rudraneel-chakraborty commented Jun 18, 2024

What is the purpose of this change?

How was this change implemented?

How was this change tested?

Is there anything the reviewers should focus on/be aware of?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CameronRushton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodiRealist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqube-solacecloud bot commented Jun 27, 2024