Skip to content

[Feature-#15103][Standalone]Support using JDBC registry plugin in standalone mode. #15139

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

Closed
wants to merge 5 commits into from

Conversation

qifanlili
Copy link

@qifanlili qifanlili commented Nov 8, 2023

Purpose of the pull request

This pull request support using JDBC registry plugin in standalone mode. close #15103

Brief change log

  • Same configuration as the Zookeeper/JDBC plugin. Type gives the user the option to use any type as a registry
  • Add README.mdl

Verify this pull request

This pull request is already covered by existing tests, such as (please describe tests).

@qifanlili qifanlili changed the title [Feature][Standalone]Support using JDBC registry plugin in standalone mode. #15103 [Feature-#15103][Standalone]Support using JDBC registry plugin in standalone mode. Nov 8, 2023
@qifanlili qifanlili marked this pull request as draft November 8, 2023 07:09
@qifanlili
Copy link
Author

qifanlili commented Nov 8, 2023

@ruanwenjun I don't know how to add labels, I'm a bit confused. Can you help me ?

@ruanwenjun ruanwenjun added the feature new feature label Nov 8, 2023
@ruanwenjun
Copy link
Member

@ruanwenjun I don't know how to add labels, I'm a bit confused. Can you help me ?

You don't have permission to add label.

@qifanlili qifanlili marked this pull request as ready for review November 8, 2023 09:51
@qifanlili
Copy link
Author

qifanlili commented Nov 8, 2023

@ruanwenjun I don't know how to add labels, I'm a bit confused. Can you help me ?

You don't have permission to add label.

@ruanwenjun Thank you so much! But I see that the check here is still not passed. Do you have to review it before it's approved?

@SbloodyS SbloodyS added first time contributor First-time contributor 3.3.0 labels Nov 8, 2023
@SbloodyS SbloodyS added this to the 3.3.0 milestone Nov 8, 2023
}
}

}
Copy link
Member

Choose a reason for hiding this comment

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

Please add a blank line here.

Copy link
Author

Choose a reason for hiding this comment

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

Please add a blank line here.

Thanks for the tip! I haven't made a habit of using “mvn spotless: apply”

public static void main(String[] args) throws Exception {
try {
// We cannot use try-with-resources to close "TestingServer", since SpringApplication.run() will not block
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Author

Choose a reason for hiding this comment

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

Why remove this?

I'm sorry I missed that. My idea is to move it to the “dolphinscheduler-standalone-server/src/main/java/org/apache/dolphinscheduler/registry/ZookeeperStandaloneRegistry.java”

qifanlili added a commit to qifanlili/dolphinscheduler that referenced this pull request Nov 8, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6175f3) 38.23% compared to head (1a240f1) 38.18%.
Report is 9 commits behind head on dev.

❗ Current head 1a240f1 differs from pull request most recent head 627427e. Consider uploading reports for the commit 627427e to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15139      +/-   ##
============================================
- Coverage     38.23%   38.18%   -0.06%     
+ Complexity     4695     4679      -16     
============================================
  Files          1285     1285              
  Lines         45453    45453              
  Branches       4951     4951              
============================================
- Hits          17381    17358      -23     
- Misses        26181    26203      +22     
- Partials       1891     1892       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…in standalone mode.(apache#15139)

fix: Add a nested comment explaining why this method is empty.
@@ -34,7 +34,7 @@
@ConditionalOnProperty(prefix = "registry", name = "type", havingValue = "jdbc")
public class JdbcRegistryConfiguration {

@Bean
@Bean("sqlSessionFactory")
Copy link
Member

Choose a reason for hiding this comment

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

It's not a good idea add name 'sqlSessionFactory' for this bean, since this will cause the JdbcRegistry use the same sql session factory with the business.

Copy link
Author

@qifanlili qifanlili Nov 9, 2023

Choose a reason for hiding this comment

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

It's not a good idea add name 'sqlSessionFactory' for this bean, since this will cause the JdbcRegistry use the same sql session factory with the business.

Thank you for reminding me of this. I write this because I find that When I using JDBCRegistry plugin,
if I don't explicitly declare 'sqlSessionFactory' , an exception will be thrown at startup:
dolphinscheduler-standalone.log image

I think it might be because of 'DaoConfiguration. java 'references it image

Is this a known problem?

Copy link
Author

Choose a reason for hiding this comment

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

@ruanwenjun can you help me? What can I do to improve this feature?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't meet this problem, it can work well when I use JDBC registry

Comment on lines 22 to 34
@Component
public class JdbcStandaloneRegistry implements StandaloneRegistry {

@Override
public void init() {
// The JDBC plugin will take effect via @ConditionalOnProperty.
}

@Override
public boolean supports(String registryType) {
return "jdbc".equalsIgnoreCase(registryType);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this? I hope we can only add a StandaloneZookeeperRegistry, in this class, we will start a TestingServer, and user can still use zookeeper registry to connect their own zk cluster or use JDBC to connect their own db.

Copy link
Author

Choose a reason for hiding this comment

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

Can we remove this? I hope we can only add a StandaloneZookeeperRegistry, in this class, we will start a TestingServer, and user can still use zookeeper registry to connect their own zk cluster or use JDBC to connect their own db.

Sure!

Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 3 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 236 Code Smells

65.7% 65.7% Coverage
1.6% 1.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

This pull request has been automatically marked as stale because it has not had recent activity for 120 days. It will be closed in 7 days if no further activity occurs.

@github-actions github-actions bot added the Stale label Mar 21, 2024
Copy link

This pull request has been closed because it has not had recent activity. You could reopen it if you try to continue your work, and anyone who are interested in it are encouraged to continue work on this pull request.

@github-actions github-actions bot closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature][Standalone] Support using JDBC registry plugin in standalone mode.
4 participants