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

fix jdbc url is not configured #923

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Conversation

FireFoxAhri
Copy link
Contributor

What is the purpose of the change

Fix a bug that JDBC engine connection url is not configured.

Related issues: #917.

Brief change log

  • Add jdbc url when it is null.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • Anything that affects deployment: (no)
  • The MGS(Microservice Governance Services), i.e., Spring Cloud Gateway, OpenFeign, Eureka.: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

val engineTypeLabel = new EngineTypeLabel()
engineTypeLabel.setEngineType("jdbc")
engineTypeLabel.setVersion("4")
RequestQueryEngineTypeDefault(engineTypeLabel)
Copy link
Member

Choose a reason for hiding this comment

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

This request can only get default configurations of jdbc engine, cannot get user configurations in governance station.
you can change to use RequestQueryEngineConfig. User and Creator labels can be found in EngineConnTask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
I've found that the jdbc url is passed as engineconn-conf, but when executing the code the conf is not used. The conf is parsed in EngineConnServer, is there any way to take the conf in TaskExecutionServiceImpl.

By the way. I've found another bug. If the jdbc url contains char like & ,eg /test?characterEncoding=UTF-8&useSSL=false, it will make the command execute in the backgroud and ignore chars behind &. Then the engineConn start failed.

Copy link
Member

Choose a reason for hiding this comment

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

image
I've found that the jdbc url is passed as engineconn-conf, but when executing the code the conf is not used. The conf is parsed in EngineConnServer, is there any way to take the conf in TaskExecutionServiceImpl.

By the way. I've found another bug. If the jdbc url contains char like & ,eg /test?characterEncoding=UTF-8&useSSL=false, it will make the command execute in the backgroud and ignore chars behind &. Then the engineConn start failed.

Thank you very much for your careful research, I hope the following answers can help you.

1.The engine's default parameters are loaded during the engine startup, such as JDBC URL, UserName, Password, etc., this parameter is the default value in the configuration, rather than user-defined values, which are invalid. So we need to dynamically load the user's configuration during the task runtime, which is the real meaning of this Pull Request.

2.When the JDBC URL is dynamically obtained via the RPC, the & Symbol should not be lost. You can debug the JDBC engine and check the createDatasources method, which will make simple processing and storage of the URL, and then establish a JDBC connection directly.

@FireFoxAhri FireFoxAhri changed the base branch from dev-1.1.0 to dev-1.0.3 November 10, 2021 08:58
Copy link
Member

@leeebai leeebai left a comment

Choose a reason for hiding this comment

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

LGTM.

@leeebai leeebai merged commit 5a7802d into apache:dev-1.0.3 Nov 10, 2021
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.

2 participants