[CDAP-21079] Refactor SPI/TMS for default context before spanner extension implementation #15731
Conversation
sidhdirenge
changed the title
cdap-messaging-spi/src/main/java/io/cdap/cdap/messaging/spi/MessagingContext.java
Outdated
Show resolved
Hide resolved
| @@ -14,7 +14,7 @@ | |||
| * the License. | |||
| */ | |||
|
|
|||
| package io.cdap.cdap.messaging.client; | |||
There was a problem hiding this comment.
Why moving it to app-fabric?
There was a problem hiding this comment.
The intention here is as follows:
-
We do not want to have dependency of (to be added) spanner extension on cdap-tms module.
-
Long term plan is to do away with the TMS pod itself. All the services will write to Spanner DB directly using the Spanner extension of messaging SPI.
-
Follow similar pattern to what data-fabric has currently : link
Please let us know what will be the best place to have this DelegatingMessagingService and the Default implementation of Messaging Context.
There was a problem hiding this comment.
IMHO, with the new messaging SPI, we have to treat tms as one implementation of messaging SPI. So it's ideal to yank out and move specific implementations to somewhere else.
@chtyim if you think it's a bit risky and prefer to keep these in TMS, fine with me.
There was a problem hiding this comment.
Based on what Masoud mentioned above, essentially there should be:
- A cdap-tms module that defines the interfaces for other modules to interact with the tms. This module also defines the guice modules for loading the correct extension based on the configuration.
- A cdap-tms-spi module that defines the interfaces for the implementation of the tms storage layer
- Bunch of cdap-tms-ext-* modules for the implementations that implements interfaces defined in the cdap-tms-spi.
All other cdap modules should just have dependency on cdap-tms and use interfaces exposed from there.
| @@ -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)); | |||
There was a problem hiding this comment.
Why move here instead of doing it in the regular initialize method?
There was a problem hiding this comment.
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().
| @@ -14,7 +14,7 @@ | |||
| * the License. | |||
| */ | |||
|
|
|||
| package io.cdap.cdap.messaging.client; | |||
|
As discussed offline about the directory structure
Tested by deploying docker image to GCP project.
All pods up and running.