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

feat: support use database as a registry #4595

Merged
merged 34 commits into from
Oct 23, 2022
Merged

Conversation

Anilople
Copy link
Contributor

@Anilople Anilople commented Oct 2, 2022

What's the purpose of this PR

Use database as a registry.

less dependency, config, startup fail,

maybe more reliability,

Which issue(s) this PR fixes:

Fixes #4583

Brief changelog

A simple try to implement a registry by database.

Eureka is still default registry, this feature is active by spring boot's profile.

Write the main code in apollo-biz module with spring boot auto configuration for configservice and adminservice.

Create a new table Registry in configdb to save the service instance's information.

There 4 column with table Registry:

  • ServiceName: the name of service. i.e apollo-configservice or apollo-adminservice.
  • Uri: instead of host and port, use an uri like http://127.0.0.1:8080/ directly, maybe it's more simple.
  • Cluster: mainly use for only discover same network region's instances.
  • Metadata: for the extension in the feature, like manually register, disable health check with instance.

Here are some concept:

  • register: save self's instance to database if it doesn't exist, update instance's dataChangeLastModifiedTime if it exists.
  • heartbeat: do register every 10s.
  • deregister: remove self's instance from database when shutdown.
  • scheduled clear policy: if an instance has no heartbeat 10 minutes, delete it from database.

Unit test, changelog and document will be added when this pr's direction is correct.

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
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #4595 (1d1d77e) into master (f8d35bd) will increase coverage by 0.75%.
The diff coverage is 76.89%.

@@             Coverage Diff              @@
##             master    #4595      +/-   ##
============================================
+ Coverage     46.48%   47.23%   +0.75%     
- Complexity     1568     1649      +81     
============================================
  Files           332      347      +15     
  Lines         10367    10631     +264     
  Branches       1043     1053      +10     
============================================
+ Hits           4819     5022     +203     
- Misses         5252     5306      +54     
- Partials        296      303       +7     
Impacted Files Coverage Δ
...llo/metaservice/controller/HomePageController.java 100.00% <ø> (ø)
.../metaservice/service/DatabaseDiscoveryService.java 0.00% <0.00%> (ø)
...o/metaservice/service/DefaultDiscoveryService.java 100.00% <ø> (ø)
...oServiceRegistryDeregisterApplicationListener.java 30.00% <30.00%> (ø)
...tion/support/ApolloServiceDiscoveryProperties.java 66.66% <66.66%> (ø)
...p/framework/apollo/biz/entity/ServiceRegistry.java 67.85% <67.85%> (ø)
...ation/support/ApolloServiceRegistryProperties.java 75.86% <75.86%> (ø)
...t/ApolloServiceRegistryClearApplicationRunner.java 81.25% <81.25%> (ø)
...tabaseDiscoveryClientMemoryCacheDecoratorImpl.java 84.61% <84.61%> (ø)
...olloServiceRegistryHeartbeatApplicationRunner.java 90.00% <90.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

BTW, the program failed to start by running com.ctrip.framework.apollo.assembly.ApolloApplication, see the error message below.
However, it works after I removed the class ApolloRegistryAutoConfiguration. What's the use of this class?

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 0 of method registryService in com.ctrip.framework.apollo.biz.registry.configuration.ApolloRegistryClientAutoConfiguration required a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' that could not be found.


Action:

Consider defining a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' in your configuration.

@Anilople
Copy link
Contributor Author

Anilople commented Oct 5, 2022

BTW, the program failed to start by running com.ctrip.framework.apollo.assembly.ApolloApplication, see the error message below. However, it works after I removed the class ApolloRegistryAutoConfiguration. What's the use of this class?

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 0 of method registryService in com.ctrip.framework.apollo.biz.registry.configuration.ApolloRegistryClientAutoConfiguration required a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' that could not be found.


Action:

Consider defining a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' in your configuration.

ApolloRegistryAutoConfiguration is used to forbid component didn't scan by springboot, see Creating a Custom Starter with Spring Boot. Write it in apollo-biz module, and conbine with conditional config, then configservice and adminservice can use it by different config.

'com.ctrip.framework.apollo.biz.repository.RegistryRepository' that could not be found

This error log still occur in my machine, I will try to fix it.

@nobodyiam
Copy link
Member

BTW, the program failed to start by running com.ctrip.framework.apollo.assembly.ApolloApplication, see the error message below. However, it works after I removed the class ApolloRegistryAutoConfiguration. What's the use of this class?

***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 0 of method registryService in com.ctrip.framework.apollo.biz.registry.configuration.ApolloRegistryClientAutoConfiguration required a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' that could not be found.


Action:

Consider defining a bean of type 'com.ctrip.framework.apollo.biz.repository.RegistryRepository' in your configuration.

ApolloRegistryAutoConfiguration is used to forbid component didn't scan by springboot, see Creating a Custom Starter with Spring Boot. Write it in apollo-biz module, and conbine with conditional config, then configservice and adminservice can use it by different config.

'com.ctrip.framework.apollo.biz.repository.RegistryRepository' that could not be found

This error log still occur in my machine, I will try to fix it.

I think ApolloRegistryAutoConfiguration is not necessary here as the ApolloRegistryClientAutoConfiguration and ApolloRegistryDiscoveryAutoConfiguration are already in the Component Scan path, which is different from a normal spring boot starter.

@Anilople
Copy link
Contributor Author

Should let user can manual register an instance that always be healthy?
i.e this instance won't remove when it cannot send heartbeat to database.

Or we just keep registry simple without the manual register function?

@nobodyiam
Copy link
Member

Should let user can manual register an instance that always be healthy? i.e this instance won't remove when it cannot send heartbeat to database.

Or we just keep registry simple without the manual register function?

I don't whether this use case is valid, is there an actual scenario?

@Anilople
Copy link
Contributor Author

Should let user can manual register an instance that always be healthy? i.e this instance won't remove when it cannot send heartbeat to database.
Or we just keep registry simple without the manual register function?

I don't whether this use case is valid, is there an actual scenario?

like profile custom-defined-discovery, register a domain, a nginx ip or a k8s service domain to database.

Some instances can be clearup, but other instances always exists whenever they have heartbeat or not.

-Dapollo.service.registry.metadata.a=1
-Dapollo.service.registry.metadata.isAutoRegister=true

generate {"a":"1","isAutoRegister":"true"} in database
@Anilople
Copy link
Contributor Author

Ok~.

I add a table column Metadata with default value {}.

if someone add config

apollo.service.registry.metadata.a=1
apollo.service.registry.metadata.isAutoRegister=true

then will generate value

{"a":"1","isAutoRegister":"true"} in database

in table's column Metadata.

Is it ok?

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.

This looks good to me now. Please help to update the CHANGES.md and add some documentation.

@Anilople
Copy link
Contributor Author

This looks good to me now. Please help to update the CHANGES.md and add some documentation.

When I write document, I notice that the file application-database-discovery.properties will be packaged in to .jar file.

If someone want to change

apollo.service.registry.cluster=default

to

apollo.service.registry.cluster=beijing-1

Is there another way to do it without rebuild the whole project?

Can we add it to config/application-database-discovery.properties in .zip file?

@nobodyiam
Copy link
Member

This looks good to me now. Please help to update the CHANGES.md and add some documentation.

When I write document, I notice that the file application-database-discovery.properties will be packaged in to .jar file.

If someone want to change

apollo.service.registry.cluster=default

to

apollo.service.registry.cluster=beijing-1

Is there another way to do it without rebuild the whole project?

Can we add it to config/application-database-discovery.properties in .zip file?

The assembly-descriptor.xml could be used to achieve this. However, I think we may simply tell the user to config apollo.service.registry.cluster in config/application-github.properties, as it's not a common use case.
And there are also many other profile-specific configs, it would look messy if we copy all of them to config/ folder.

image

@Anilople
Copy link
Contributor Author

This looks good to me now. Please help to update the CHANGES.md and add some documentation.

When I write document, I notice that the file application-database-discovery.properties will be packaged in to .jar file.
If someone want to change

apollo.service.registry.cluster=default

to

apollo.service.registry.cluster=beijing-1

Is there another way to do it without rebuild the whole project?
Can we add it to config/application-database-discovery.properties in .zip file?

The assembly-descriptor.xml could be used to achieve this. However, I think we may simply tell the user to config apollo.service.registry.cluster in config/application-github.properties, as it's not a common use case. And there are also many other profile-specific configs, it would look messy if we copy all of them to config/ folder.

image

Agree that.

it would look messy if we copy all of them to config/ folder.

And there is another thing about spring.profiles.active, I opened a discussion #4610

@Anilople
Copy link
Contributor Author

There are some feature need to support later:

  • use jvm memory as cache to decrease the read of database.
  • when database happened failure, config service can run well.

left them in another pr?

@nobodyiam
Copy link
Member

  1. use jvm memory as cache to decrease the read of database.
  2. when database happened failure, config service can run well.

I think we could use the first feature to enable the second feature, e.g. when the database failed, we could still use the service instance in the memory cache.
This could be enhanced with another pr.

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

@nobodyiam nobodyiam merged commit 22e6422 into master Oct 23, 2022
@nobodyiam nobodyiam deleted the feature/register-to-database branch October 23, 2022 10:30
@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support config service and admin service use mysql as registry
2 participants