(Abandoned) Please go to #2970 PR. by 743294668 · Pull Request #2832 · apache/dolphinscheduler

743294668 · 2020-05-28T03:31:27Z

What is the purpose of the pull request

This pull request modify the JDBC format of the Oracle database connection.
Number:#2791.
pull/2792 ，pull/2818has been abandoned for other reasons.

The Oracle JDBC link I verified currently supports the following three methods:
jdbc:oracle:thin:@host:port:SID
jdbc:oracle:thin:@//host:port/service_name
jdbc:oracle:thin:@host:port/service_name
The above three methods are verified in the unit test.

Brief change log

DataSourceService.java:
Add Oracle link type parameter in buildParameter().
BaseDataSource.java:
Modify appendDatabase () method as a protected modifier to facilitate OracleDataSource to rewrite this method.
OracleDataSource.java:
Override appendDatabase () method.
OracleDataSourceTest.java:
Unit test the newly modified code.
DataSourceServiceTest.java:
Add buildParameter () unit test.
pom.xml:
added a line in the maven-surefire-plugin section of the pom file to ensure that my OracleDataSourceTest.java file can be executed, thus ensuring my code coverage.

Verify this pull request

This change added tests and can be verified as follows:

Add the Oracle data source in the data source center, including using the service name or SID.
Perform the task of executing the Oracle data source on the SQL type node and pass the verification.
##Addition

If anything is wrong, you are welcome to participate in the discussion. Thanks.

codecov-commenter · 2020-05-28T03:40:22Z

Codecov Report

Merging #2832 into dev will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                dev    #2832      +/-   ##
============================================
+ Coverage     37.19%   37.35%   +0.16%     
- Complexity     2559     2576      +17     
============================================
  Files           435      435              
  Lines         20020    20022       +2     
  Branches       2426     2425       -1     
============================================
+ Hits           7447     7480      +33     
+ Misses        11907    11858      -49     
- Partials        666      684      +18

Impacted Files	Coverage Δ	Complexity Δ
...olphinscheduler/dao/datasource/BaseDataSource.java	`50.00% <ø> (+50.00%)`	`15.00 <0.00> (+15.00)`
...olphinscheduler/api/service/DataSourceService.java	`7.27% <100.00%> (+5.05%)`	`5.00 <0.00> (+2.00)`
...phinscheduler/dao/datasource/OracleDataSource.java	`90.90% <100.00%> (+90.90%)`	`6.00 <2.00> (+6.00)`
.../server/worker/processor/TaskExecuteProcessor.java	`10.16% <0.00%> (-27.12%)`	`1.00% <0.00%> (-2.00%)`
...nscheduler/server/entity/TaskExecutionContext.java	`81.18% <0.00%> (-5.95%)`	`57.00% <0.00%> (-3.00%)`
...e/dolphinscheduler/remote/NettyRemotingClient.java	`51.07% <0.00%> (-2.88%)`	`9.00% <0.00%> (-2.00%)`
...pache/dolphinscheduler/common/utils/FileUtils.java	`28.97% <0.00%> (+0.93%)`	`9.00% <0.00%> (ø%)`
...cheduler/server/registry/ZookeeperNodeManager.java	`78.02% <0.00%> (+3.29%)`	`7.00% <0.00%> (+1.00%)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12c249a...06aca8e. Read the comment docs.

sonarqubecloud · 2020-06-05T02:54:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

100.0% Coverage
0.0% Duplication

The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

gabry-lab

It's better not to change the scope of appendDatabase. You can override the getJdbcUrl directly

743294668 · 2020-06-05T06:38:58Z

It's better not to change the scope of appendDatabase. You can override the getJdbcUrl directly

At first I did getJdbcUrl modified, and then I noticed #2812 modification, I made changes based on this basis, and covered appendDatabase.
For now, do you want to rewrite getJdbcUrl instead? Any other suggestions? Looking forward for your response, thank you

gabry-lab · 2020-06-05T06:43:39Z

It's better not to change the scope of appendDatabase. You can override the getJdbcUrl directly

At first I did getJdbcUrl modified, and then I noticed #2812 modification, I made changes based on this basis, and covered appendDatabase.
For now, do you want to rewrite getJdbcUrl instead? Any other suggestions? Looking forward for your response, thank you

Oh dev-1.3.0 branch the getJdbcUrl method removed, maybe we should invite @lgcareer to review this PR

743294668 · 2020-06-08T01:47:51Z

@lgcareer can you help review this? Thank you. Please explain if there is any problem.

lgcareer · 2020-06-08T10:02:50Z

@lgcareer can you help review this? Thank you. Please explain if there is any problem.

lgcareer · 2020-06-08T10:05:41Z

@lgcareer can you help review this? Thank you. Please explain if there is any problem.

You can test use "/" before sid,such as jdbc:oracle:thin:@//:1521/SID,I think we didn't must use ":" before sid.

743294668 · 2020-06-09T02:36:32Z

@lgcareer can you help review this? Thank you. Please explain if there is any problem.

You can test use "/" before sid,such as jdbc:oracle:thin:@//:1521/SID,I think we didn't must use ":" before sid.

Regarding the format of the URL, I am pretty sure that I will list it again. Before submitting this code, I used the local mode to run the API module test. Only the above three methods can pass. Regarding the question you mentioned, I conducted a query on Google again, and I did not find this way of using "/" to connect sid. It may be that the author's sid and service_name have the same name.
look forward to your reply. Thank you.

lgcareer · 2020-06-09T02:59:11Z

@lgcareer can you help review this? Thank you. Please explain if there is any problem.

You can test use "/" before sid,such as jdbc:oracle:thin:@//:1521/SID,I think we didn't must use ":" before sid.

Regarding the format of the URL, I am pretty sure that I will list it again. Before submitting this code, I used the local mode to run the API module test. Only the above three methods can pass. Regarding the question you mentioned, I conducted a query on Google again, and I did not find this way of using "/" to connect sid. It may be that the author's sid and service_name have the same name.
look forward to your reply. Thank you.

If you test it and it can't work successful,I think you can use your code instead of I said,May be you are right,The last time I test successful is that the service_name and SID is consistent.

743294668 · 2020-06-09T03:11:15Z

@gabrywu can you help review this? Thank you. Now I don’t know who I should check with, because I see that you did a requested changes.
Please explain if there is any problem.

lgcareer

+1

743294668 · 2020-06-10T10:14:36Z

It's better not to change the scope of appendDatabase. You can override the getJdbcUrl directly

lgcareer has reviewed the code, this method is no problem. The restriction also requires you to modify and review it, thank you

gabry-lab

+1

gabry-lab · 2020-06-11T01:53:08Z

It's better not to change the scope of appendDatabase. You can override the getJdbcUrl directly

lgcareer has reviewed the code, this method is no problem. The restriction also requires you to modify and review it, thank you

done. thanks for your contribution

Jave-Chen

+1

gabry-lab · 2020-06-14T02:31:32Z

@743294668 Because the original branch of your repo , this PR can't be merged , so could you create another branch and PR to resolve this ?

743294668 · 2020-06-14T09:26:57Z

@743294668 Because the original branch of your repo , this PR can't be merged , so could you create another branch and PR to resolve this ?

Thanks for your reminder. I resubmitted a PR at #2970.

gabry-lab · 2020-06-15T03:12:45Z

@743294668 Because the original branch of your repo , this PR can't be merged , so could you create another branch and PR to resolve this ?

Thanks for your reminder. I resubmitted a PR at #2970.

merged

Modify OracleDB connection string format.

f4aba27

743294668 added 2 commits June 1, 2020 08:36

Merge branch 'dev' into Thomas

f0d2099

Merge branch 'dev' into Thomas

06aca8e

gabry-lab suggested changes Jun 5, 2020

View reviewed changes

lgcareer approved these changes Jun 9, 2020

View reviewed changes

743294668 requested a review from gabry-lab June 9, 2020 08:45

gabry-lab approved these changes Jun 11, 2020

View reviewed changes

Jave-Chen approved these changes Jun 13, 2020

View reviewed changes

743294668 mentioned this pull request Jun 14, 2020

Alternative #2832 Pull requests. #2970

Merged

743294668 changed the title ~~Modify OracleDB connection string format.~~ (Abandoned) Please go to #2970 PR. Jun 15, 2020

gabry-lab closed this Jun 15, 2020

Conversation

743294668 commented May 28, 2020

What is the purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

codecov-commenter commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Jun 5, 2020

Uh oh!

gabry-lab left a comment

Choose a reason for hiding this comment

Uh oh!

743294668 commented Jun 5, 2020

Uh oh!

gabry-lab commented Jun 5, 2020

Uh oh!

743294668 commented Jun 8, 2020

Uh oh!

lgcareer commented Jun 8, 2020

Uh oh!

lgcareer commented Jun 8, 2020

Uh oh!

743294668 commented Jun 9, 2020

Uh oh!

lgcareer commented Jun 9, 2020

Uh oh!

743294668 commented Jun 9, 2020

Uh oh!

lgcareer left a comment

Choose a reason for hiding this comment

Uh oh!

743294668 commented Jun 10, 2020

Uh oh!

gabry-lab left a comment

Choose a reason for hiding this comment

Uh oh!

gabry-lab commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jave-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

gabry-lab commented Jun 14, 2020

Uh oh!

743294668 commented Jun 14, 2020

Uh oh!

gabry-lab commented Jun 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented May 28, 2020 •

edited

Loading

gabry-lab commented Jun 11, 2020 •

edited

Loading