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.

Sign up for GitHub

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

Jump to bottom

apollo assembly optimization #5035

Merged
merged 70 commits into from
Feb 3, 2024
Merged

apollo assembly optimization #5035

merged 70 commits into from
Feb 3, 2024

Conversation

vdiskg
Copy link
Contributor

@vdiskg vdiskg commented Dec 6, 2023

What's the purpose of this PR

  1. Assembly Portal, Config Service, Admin Service into a standalone process
  2. Assembly ApolloConfigDB, ApolloPortalDB into a single db.

Which issue(s) this PR fixes:

#4313
#4729

Brief changelog

  1. add table name prefix in the assembly sql
  2. add TablePrefixNamingStrategy to match the table name prefix

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@codecov Codecov
Copy link

codecov bot commented Dec 6, 2023 •
edited
Loading

Codecov Report

Attention: 240 lines in your changes are missing coverage. Please review.

Comparison is base (921af84) 49.65% compared to head (e8b44ee) 50.38%.

Files Patch % Lines
...lloDataSourceScriptDatabaseInitializerFactory.java 0.00% 75 Missing ⚠️
...lo/build/sql/converter/ApolloSqlConverterUtil.java 77.72% 35 Missing and 12 partials ⚠️
.../datasource/ApolloSqlInitializationProperties.java 0.00% 32 Missing ⚠️
...rce/ApolloDataSourceScriptDatabaseInitializer.java 0.00% 25 Missing ⚠️
...ework/apollo/build/sql/converter/SqlStatement.java 55.55% 10 Missing and 2 partials ⚠️
...llo/build/sql/converter/ApolloH2ConverterUtil.java 91.52% 6 Missing and 4 partials ⚠️
...apollo/build/sql/converter/ApolloSqlConverter.java 86.36% 9 Missing ⚠️
...apollo/build/sql/converter/SqlTemplateContext.java 62.50% 6 Missing ⚠️
...rk/apollo/build/sql/converter/SqlTemplateGist.java 82.35% 6 Missing ⚠️
...sql/converter/ApolloMysqlDefaultConverterUtil.java 78.26% 4 Missing and 1 partial ⚠️
... and 7 more
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #5035      +/-   ##
============================================
+ Coverage     49.65%   50.38%   +0.72%     
- Complexity     1908     2012     +104     
============================================
  Files           372      387      +15     
  Lines         11564    12213     +649     
  Branches       1127     1207      +80     
============================================
+ Hits           5742     6153     +411     
- Misses         5479     5697     +218     
- Partials        343      363      +20

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

@nobodyiam
Copy link
Member

Wow! This pr is truly thrilling!

@vdiskg vdiskg marked this pull request as ready for review December 14, 2023 14:05
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Dec 14, 2023
nobodyiam
nobodyiam reviewed Dec 18, 2023
View reviewed changes
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Now the whole apollo servers could be launched in one process, this is amazing!
Would you please also update the apollo-development-guide.md?

image

...ice/src/main/java/com/ctrip/framework/apollo/adminservice/AdminServiceAutoConfiguration.java Outdated Show resolved Hide resolved
apollo-assembly/src/main/resources/application-github.properties Outdated Show resolved Hide resolved
docs/en/deployment/quick-start.md
No configuration is required, just use the following command to start
> Note: When using the in-memory database, any operation will be lost after the Apollo process restarts
```bash
export SPRING_PROFILES_ACTIVE="github,auth"
Copy link
Member

Choose a reason for hiding this comment

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

Is it feasible to enhance the demo.sh script to enable users to initiate the quick-start demonstration efficiently through some configuration settings?

@nobodyiam
Copy link
Member

nobodyiam commented Dec 18, 2023 •
edited
Loading

It appears there is some incompatibility with database-discovery. When I execute the assembly using the spring profiles github,auth,database-discovery, only the apollo-configservice gets registered.

image

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 12, 2024
View reviewed changes
...src/main/java/com/ctrip/framework/apollo/adminservice/AdminServiceAssemblyConfiguration.java

@Override
protected void configure(HttpSecurity http) throws Exception {
http.csrf().disable();

Check failure

Code scanning / CodeQL

Disabled Spring CSRF protection High

CSRF vulnerability due to protection being disabled.
@nobodyiam
Copy link
Member

This pull request appears to be in good shape now. Could you also update the CHANGES.md file? Since this constitutes a major change, it's important to document it there.

@nobodyiam
Copy link
Member

BTW, #5072 was merged with a schema change, would you please help to update the code base again?

nobodyiam
nobodyiam reviewed Jan 27, 2024
View reviewed changes
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

Please also fix the failed tests.

2024-01-26T16:03:31.8898714Z Caused by: java.lang.IllegalStateException: @order on WebSecurityConfigurers must be unique. Order of 99 was already used on com.ctrip.framework.apollo.adminservice.controller.TestWebSecurityConfig$$EnhancerBySpringCGLIB$$ac5b1e3@469959b6, so it cannot be used on com.ctrip.framework.apollo.adminservice.AdminServiceAssemblyConfiguration$AdminServiceSecurityConfigurer$$EnhancerBySpringCGLIB$$8154145d@270147e3 too.

apollo-assembly/src/main/resources/logback.xml Outdated Show resolved Hide resolved
@vdiskg
clean logger
4d72047
@vdiskg
Merge branch 'master' into assembly
a8398e5 
# Conflicts:
#	docs/en/contribution/apollo-development-guide.md
#	docs/zh/contribution/apollo-development-guide.md
@vdiskg
fix order
4941f4f
@vdiskg
fix h2 converter
06afabb
@vdiskg
fix h2 converter v0.2
d68ed97
@vdiskg
fix order v0.2
e8b44ee
nobodyiam
nobodyiam approved these changes Feb 3, 2024
View reviewed changes
Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 3, 2024
@nobodyiam nobodyiam merged commit c90c930 into apolloconfig:master Feb 3, 2024
11 of 12 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
@nobodyiam nobodyiam added this to the 2.3.0 milestone Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nobodyiam nobodyiam nobodyiam approved these changes

Assignees
No one assigned
Labels
lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@vdiskg @nobodyiam