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

TH2-4267 split common #232

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

TH2-4267 split common #232

wants to merge 6 commits into from

Conversation

Xanclry
Copy link
Contributor

@Xanclry Xanclry commented Oct 4, 2022

No description provided.

@@ -38,6 +38,7 @@ ext {

repositories {
mavenCentral()
mavenLocal()
Copy link
Member

Choose a reason for hiding this comment

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

Should be the last repo in the list

Choose a reason for hiding this comment

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

I don't agree. I think it will be first or won't be because if you build project via clean build pTML gradle publishes artifact as release.
If we want to write mavenLocal() in the build.gradle script it should be first otherwise we shouldn't be specified

build.gradle Outdated Show resolved Hide resolved
@Xanclry Xanclry requested a review from OptimumCode December 5, 2022 12:31
Comment on lines +36 to +38
ServiceLoader.load(ModuleFactory::class.java).forEach { moduleFactory ->
modulesFactoryMapping[moduleFactory.moduleType] = moduleFactory
}
Copy link
Member

Choose a reason for hiding this comment

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

Why modules are in the configuration manager? Shouldn't they be in the factory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class has already cached configurations, so I think it's better to put all caching in one place

Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this cache into an abstract part of ConfigurationProvider CommonFactory to avoid new entities

Choose a reason for hiding this comment

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

After discussion with @Xanclry we have decided to rename ConfigurationManager to ModuleManager


+ Dynamic structure of common
+ Extracted common/cassandra to separate module
+ Implemented api for dynamic loading of modules and configurations

Choose a reason for hiding this comment

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

Suggested change
+ Implemented api for dynamic loading of modules and configurations
+ Implemented the API for dynamic loading of modules and configurations

Choose a reason for hiding this comment

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

Also we should describe module API and how it works in a real project

Comment on lines +23 to +51
@Parameter(names = ["-c", "--configs"])
lateinit var config: String

@Parameter(names = ["--rabbitConfiguration"])
lateinit var rabbitConfiguration: String

@Parameter(names = ["--messageRouterConfiguration"])
lateinit var messageRouterConfiguration: String

@Parameter(names = ["--grpcRouterConfiguration"])
lateinit var grpcRouterConfiguration: String

@Parameter(names = ["--grpcConfiguration"])
lateinit var grpcConfiguration: String

@Parameter(names = ["--grpcRouterConfig"])
lateinit var grpcRouterConfig: String

@Parameter(names = ["--customConfiguration"])
lateinit var customConfiguration: String

@Parameter(names = ["--dictionariesDir"])
lateinit var dictionariesDir: String

@Parameter(names = ["--prometheusConfiguration"])
lateinit var prometheusConfiguration: String

@Parameter(names = ["--boxConfiguration"])
lateinit var boxConfiguration: String

Choose a reason for hiding this comment

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

Remove these properties, please use hard coded names for them

Comment on lines +36 to +38
ServiceLoader.load(ModuleFactory::class.java).forEach { moduleFactory ->
modulesFactoryMapping[moduleFactory.moduleType] = moduleFactory
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this cache into an abstract part of ConfigurationProvider CommonFactory to avoid new entities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants