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

[CDAP-21079] Refactor SPI/TMS for default context before spanner extension implementation #15731

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

sidhdirenge
Copy link
Contributor

@sidhdirenge sidhdirenge commented Nov 11, 2024

  • Added MessagingContext and its default impl.
  • Added initialize() in MessagingService interface to initialize the MessagingContext based on cconf.

Tested by deploying docker image to GCP project.
All pods up and running.

@sidhdirenge sidhdirenge added the build Triggers github actions build label Nov 11, 2024
@sidhdirenge sidhdirenge changed the title Refactor SPI/TMS for default context before spanner extension implementation [CDAP-21079] Refactor SPI/TMS for default context before spanner extension implementation Nov 11, 2024
@sidhdirenge sidhdirenge marked this pull request as ready for review November 13, 2024 17:34
@@ -130,6 +135,11 @@ private MessagingService getDelegate() {
"Unsupported messaging service implementation " + getName());
}
LOG.info("Messaging service {} is loaded", messagingService.getName());
try {
messagingService.initialize(new DefaultMessagingContext(this.cConf));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move here instead of doing it in the regular initialize method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Messaging currently doesn't have initialize().
We need this to create spanner database clients. Extension is loaded at this particular place and the current object's delegate is set to the messagingService. So thought this would be an appropriate place for initialize().

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@sidhdirenge sidhdirenge requested a review from chtyim November 14, 2024 20:23
@sidhdirenge sidhdirenge added build Triggers github actions build and removed build Triggers github actions build labels Nov 15, 2024
Copy link
Contributor

@masoud-io masoud-io left a comment

Choose a reason for hiding this comment

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

Minor comment; Otherwise LGTM

@sidhdirenge sidhdirenge force-pushed the sidhdirenge-refactor-messaging-spi branch from 4901eea to 3e37e7d Compare November 19, 2024 09:04
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@sidhdirenge sidhdirenge merged commit 1cf80b7 into develop Nov 19, 2024
10 of 11 checks passed
@sidhdirenge sidhdirenge deleted the sidhdirenge-refactor-messaging-spi branch November 19, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Triggers github actions build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants