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

Instead of datasource-settings.properties, use application.yml for MySQLStorageProvider #3564

Merged
merged 29 commits into from
Oct 12, 2019
Merged

Conversation

tristaZero
Copy link
Contributor

@tristaZero tristaZero commented Oct 6, 2019

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

  • How to fix?
    1.Create DynamicModuleConfig
    2.Create MySQLStorageConfig
    3.Modify ModuleDefine for using ModuleConfiguration.
    4.Add all properties of HikariCP to application.yml
    5.Delete datasource-settings.properties


New feature or improvement

  • Describe the details and related test reports.

@tristaZero tristaZero changed the title Instead of datasource-settings.properties, use application.yaml for MySQLStorageProvider Instead of datasource-settings.properties, use application.yml for MySQLStorageProvider Oct 6, 2019
@wu-sheng wu-sheng added feature New feature backend OAP backend related. labels Oct 6, 2019
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

I think, you should make Module configuration injection to support properties or map. Using DynamicModuleConfig has a limit, if you have two properties typd fields.

And making field type extension should be more elegance.
Make sense?

And you should raise a MySQL e2e PR to across verify this.

Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

See comments inline, and the term Dynamic Config has special meaning in SkyWalking, so I think DynamicModuleConfig may be confusing to our users, they may think that the datasource configurations can be dynamically modified at runtime, which is not the case.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 6, 2019

@tristaZero read the dynamic configuration in backend advanced feature. You will find out why.

@tristaZero
Copy link
Contributor Author

I think, you should make Module configuration injection to support properties or map. Using DynamicModuleConfig has a limit, if you have two properties typd fields.

And making field type extension should be more elegance.
Make sense?

And you should raise a MySQL e2e PR to across verify this.

What sceneria two properties is necessary? Why don't we put the settings from two properties into one properties?

@wu-sheng
Copy link
Member

wu-sheng commented Oct 6, 2019

I said this in design level. ModuleConfig injection is one of oap core features. This is field type triggered, so when you want properties field, I prefer this way.

For example, you could support properties to match the yaml map type.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 6, 2019

This PR could be separated into 3 PRs

  1. Config injection supports map and properties type
  2. New MySQL config implementation
  3. e2e for mysql storage.

@wu-sheng
Copy link
Member

wu-sheng commented Oct 6, 2019

One remind, once you change the mysql config, you need to change this too.

generateStorageMySQL() {
cat <<EOT >> ${var_application_file}
storage:
mysql:
metadataQueryMaxSize: \${SW_STORAGE_H2_QUERY_MAX_SIZE:5000}
EOT
}

FYI @kezhenxu94 Do you have any idea, how to make the docker-compose checked in CI?

@tristaZero
Copy link
Contributor Author

I said this in design level. ModuleConfig injection is one of oap core features. This is field type triggered, so when you want properties field, I prefer this way.

For example, you could support properties to match the yaml map type.

The properties of one ModuleConfig, e.g, MySQLModuleConfig, set in application.yml are like this way:

storege:
  mysql:
    jdbcUrl: ${SW_JDBC_URL:"jdbc:mysql://localhost:3306/swtest"}
    dataSource.user: ${SW_DATA_SOURCE_USER:root}
    dataSource.password: ${SW_DATA_SOURCE_PASSWORD:root@1234}
    dataSource.cachePrepStmts: ${SW_DATA_SOURCE_CACHE_PREP_STMTS:true}
    dataSource.prepStmtCacheSize: ${SW_DATA_SOURCE_PREP_STMT_CACHE_SQL_SIZE:250}
    dataSource.prepStmtCacheSqlLimit: ${SW_DATA_SOURCE_PREP_STMT_CACHE_SQL_LIMIT:2048}
    dataSource.useServerPrepStmts: ${SW_DATA_SOURCE_USE_SERVER_PREP_STMTS:true}
    metadataQueryMaxSize: ${SW_STORAGE_H2_QUERY_MAX_SIZE:5000}

So one properties can contain all the settings below mysql:, do we need another properties? Fow what?

@wu-sheng
Copy link
Member

wu-sheng commented Oct 6, 2019

I am not saying in MySQL provider, you need two or more properties fields. Clearly you don't.
But you should take a look at what packages are you placing your implementation

  • org.apache.skywalking.oap.server.library.module.DynamicModuleConfig
  • org.apache.skywalking.oap.server.library.module. ModuleDefine

Those two are in core module, which means, being satisfied for MySQL implementation is not enough. That is what I mean in design level. You never know which provider will use two properties, even sometimes they are not open source implementation in the upstream.

So, once we want to support properties type, which makes totally sense to me, we should bring a good and elegance feature in the core.

Is this clear for you now?

@wu-sheng
Copy link
Member

wu-sheng commented Oct 10, 2019

Do we have two datasource-settings.properties files? I remember so. Only find one deleted, please confirm.


Verified, yes, there are two files. You seem to miss one.

@kezhenxu94
Copy link
Member

@tristaZero and it seems that you forgot the reminder above

@tristaZero
Copy link
Contributor Author

Do we have two datasource-settings.properties files? I remember so. Only find one deleted, please confirm.

Thanks for your reminder, i deleted it.

@tristaZero
Copy link
Contributor Author

@tristaZero and it seems that you forgot the reminder above

Thanks, fixed it.

@wu-sheng wu-sheng added this to the 6.5.0 milestone Oct 12, 2019
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Basically, this PR LGTM. @kezhenxu94 What do you think?

I expect a new e2e test case added to test MySQL storage, but before doing that, I hope you could separate our e2e into 2 groups rather than 4, even 5(if MySQL added), which cost unpredictable CPU in CI env. Agree?

@tristaZero
Copy link
Contributor Author

Basically, this PR LGTM. @kezhenxu94 What do you think?

I expect a new e2e test case added to test MySQL storage, but before doing that, I hope you could separate our e2e into 2 groups rather than 4, even 5(if MySQL added), which cost unpredictable CPU in CI env. Agree?

Is #3583 the same one with what your mentioned?

@wu-sheng
Copy link
Member

Is #3583 the same one with what your mentioned?

That is only plugin test. I mean e2e test, check test/e2e folder and this doc
https://github.com/apache/skywalking/blob/master/docs/en/guides/README.md#end-to-end-tests-e2e-for-short

kezhenxu94
kezhenxu94 previously approved these changes Oct 12, 2019
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

@wu-sheng This PR looks good to me too now, if we don't expect to include an (MySQL) e2e test in this PR, I'm ok to merge this, and I'm slightly refactor the e2e running script in my ES7 adaptation PR, as what you expect (2 groups)

@wu-sheng
Copy link
Member

I will wait for the unit test for the new properties field, then I think the MySQL e2e could start in another PR.

@wu-sheng
Copy link
Member

I'm slightly refactor the e2e running script in my ES7 adaptation PR, as what you expect (2 groups)

Sure. Look forward to your new PR about this feature. I think the community will like this.

@kezhenxu94 kezhenxu94 dismissed their stale review October 12, 2019 07:34

Wait for the unit tests

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM. Look forward to your e2e PR.

@wu-sheng wu-sheng merged commit fb00186 into apache:master Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants