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

Optimize the message of DatabaseServerInfo #28428

Merged
merged 4 commits into from
Sep 15, 2023
Merged

Conversation

134130
Copy link
Contributor

@134130 134130 commented Sep 13, 2023

Fixes #28421.

in: proxy type: refactor

Changes proposed in this pull request:

  • The "Database name" is ambiguous; It can be DBMS's product name or It's inner schema's name.
  • In normally, we don't call MySQL as "DBMS product name" or "DBMS name". Just "DBMS" is MySQL.
  • Just DBMS is clear.
  • Not changing inner field's naming; Since it is already clear that DatabaseServer's name means DatabaseProduct's name

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

@134130 134130 marked this pull request as ready for review September 13, 2023 08:35
@RaigorJiang
Copy link
Contributor

Hi @134130
Thank you for your serious reply.

In ShardingSphere, we use database type to describe the type of database.
Do you think it is okay to change the log to Database type is xx?

Please refer to:

@134130
Copy link
Contributor Author

134130 commented Sep 13, 2023

@RaigorJiang Yeah, I've considered using database type since many fields and arguments are named with it.
But when we saying database type, It means such as type of DBMSs which like RDBMS, NoSQL, Hierarchical DB, Data Warehouse and somethings...
So I thought using DBMS is better for on message case.

But I am open to changing message to Database type ;)

@RaigorJiang
Copy link
Contributor

@RaigorJiang Yeah, I've considered using database type since many fields and arguments are named with it. But when we saying database type, It means such as type of DBMSs which like RDBMS, NoSQL, Hierarchical DB, Data Warehouse and somethings... So I thought using DBMS is better for on message case.

But I am open to changing message to Database type ;)

In ShardingSphere's Design Philosophy, SQL/NoSQL/NewSQL are considered heterogeneous databases, so can they be collectively called databases?

image

@134130
Copy link
Contributor Author

134130 commented Sep 13, 2023

Since ShardingSphere uses Database Product name as Database type in many codes, I also agree concentrate
to Database type.

In ShardingSphere's Design Philosophy, SQL/NoSQL/NewSQL are considered heterogeneous databases, so can they be collectively called databases?

Are you saying database only means Relational databases? I think "Heterogeneous databases" contains all of SQL NoSQL and Others which can stores data.
And I suggests Database kind or Database class to dividing SQL and NoSQL later; Haskell uses Kind as a type of type 😃

@134130
Copy link
Contributor Author

134130 commented Sep 13, 2023

Do this common class really used in only 3 cases(including 2 tests)?? 😂
image

@RaigorJiang
Copy link
Contributor

Do this common class really used in only 3 cases(including 2 tests)?? 😂

It seems so.
Some ci failed, please merge the master branch to solve the problem, thank you!

@RaigorJiang RaigorJiang added this to the 5.4.1 milestone Sep 14, 2023
@RaigorJiang RaigorJiang merged commit 871cedb into apache:master Sep 15, 2023
127 checks passed
@RaigorJiang
Copy link
Contributor

RaigorJiang commented Sep 15, 2023

@134130 Merged, thank you!

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

Successfully merging this pull request may close these issues.

Logging: optimize the message of database name.
2 participants