DATAGO-79772: Cloud managed EMA must not stream configPush logs to EP Core #191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rudraneel-chakraborty
had a problem deploying
to
build_pr
GitHub Actions
Failure
rudraneel-chakraborty
had a problem deploying
to
build_pr
GitHub Actions
Failure
Comment on lines
-53
to
-73
| public void deleteExecutionLogFiles(List<Path> listOfExecutionLogFiles) { | ||
| boolean allFilesDeleted = listOfExecutionLogFiles | ||
| .stream() | ||
| .allMatch(this::deleteExecutionLogFile); | ||
| if (!allFilesDeleted) { | ||
| throw new IllegalArgumentException("Some of the execution log files were not deleted. Please check the logs"); | ||
| } | ||
| } | ||
|
|
||
| private boolean deleteExecutionLogFile(Path path) { | ||
| try { | ||
| if (Files.exists(path)) { | ||
| Files.delete(path); | ||
| } | ||
| } catch (IOException e) { | ||
| log.warn("Error while deleting execution log at {}", path, e); | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Extracted these methods as is to CommandManager as these methods are better suited there
Comment on lines
-164
to
-177
| @Bean | ||
| @Qualifier("realCommandLogStreamingProcessor") | ||
| public CommandLogStreamingProcessor realCommandLogStreamingProcessor(CommandLogsPublisher commandLogsPublisher, | ||
| EventPortalProperties eventPortalProperties, | ||
| ObjectMapper objectMapper) { | ||
| return new CommandLogStreamingProcessor(commandLogsPublisher, eventPortalProperties, objectMapper); | ||
| } | ||
|
|
||
| @Bean | ||
| @Qualifier("mockedCommandLogStreamingProcessor") | ||
| @Primary | ||
| public CommandLogStreamingProcessor mockedCommandLogStreamingProcessor() { | ||
| return mock(CommandLogStreamingProcessor.class); | ||
| } |
There was a problem hiding this comment.
Not needed
| @@ -45,14 +48,14 @@ public class CommandManager { | |||
| private final MessagingServiceDelegateService messagingServiceDelegateService; | |||
| private final EventPortalProperties eventPortalProperties; | |||
| private final ThreadPoolTaskExecutor configPushPool; | |||
| private final CommandLogStreamingProcessor commandLogStreamingProcessor; | |||
| private final Optional<CommandLogStreamingProcessor> commandLogStreamingProcessorOpt; | |||
There was a problem hiding this comment.
commandLogStreamingProcessor is absent for cloud managed mode. So making it optional
Comment on lines
+112
to
+114
| if (commandLogStreamingProcessorOpt.isPresent()) { | ||
| streamCommandExecutionLogToEpCore(request, command, executionLog); | ||
| } |
There was a problem hiding this comment.
Only stream if the commandLogStreamingProcessorOpt is present (e.g. in self managed mode)
rudraneel-chakraborty
had a problem deploying
to
build_pr
GitHub Actions
Failure
rudraneel-chakraborty
changed the title
rudraneel-chakraborty
temporarily deployed
to
build_pr
GitHub Actions
Inactive
| @DirtiesContext(classMode = BEFORE_EACH_TEST_METHOD) | ||
| //CPD-OFF | ||
| class CloudManagedEMACommandManagerTests { | ||
|
|
There was a problem hiding this comment.
This needs to be a separate class as we want to load a new application context in cloud managed mode
moodiRealist
reviewed
Jul 8, 2024
| id: gateway | ||
| name: evmr1 | ||
| id: f0ydyzsr1jp | ||
| name: POCCloudManagedEMAEVMR |
There was a problem hiding this comment.
Please undo these changes.
There was a problem hiding this comment.
Done. See slack message on how I mitigated the impact
rudraneel-chakraborty
temporarily deployed
to
build_pr
GitHub Actions
Inactive
moodiRealist
reviewed
Jul 8, 2024
| @@ -37,6 +38,9 @@ public class TerraformManager { | |||
| private final TerraformProperties terraformProperties; | |||
| private final TerraformClientFactory terraformClientFactory; | |||
|
|
|||
|
|
|||
| private boolean isManagedAgent; | |||
There was a problem hiding this comment.
Where is this boolean used?
There was a problem hiding this comment.
Removed. Good Catch
rudraneel-chakraborty
temporarily deployed
to
build_pr
GitHub Actions
Inactive
moodiRealist
reviewed
Jul 9, 2024
| @@ -7,6 +7,7 @@ | |||
| import com.solace.maas.ep.event.management.agent.plugin.command.model.CommandResult; | |||
| import com.solace.maas.ep.event.management.agent.plugin.command.model.JobStatus; | |||
| import com.solace.maas.ep.event.management.agent.plugin.constants.RouteConstants; | |||
| import com.solace.maas.ep.event.management.agent.plugin.messagingService.event.EventProperty; | |||
There was a problem hiding this comment.
I guess this library doesn't have PMD checks. Otherwise, it should have failed the build for un-used imports.
Please just roll-back this file considering there is no relevant change here.
rudraneel-chakraborty
temporarily deployed
to
build_pr
GitHub Actions
Inactive
moodiRealist
approved these changes
Jul 10, 2024
|
SonarQube Quality Gate
|
rudraneel-chakraborty
deleted the
DATAGO-79772-no-log-streaming-to-ep-for-cema
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of this change?
Currently, Self managed EMA streams Terraform execution logs to ep-core. These logs end up in Datadog and aids in debuggability. Cloud managed EMA already has DD integration. So we intend to disable the log streaming feature if the ema is running in cloud managed mode
How was this change implemented?
Java code change
How was this change tested?
IT
Is there anything the reviewers should focus on/be aware of?