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

Change to use shouldFetchRegistry. #3627

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aftersss
Copy link

@aftersss aftersss commented Aug 23, 2019

When I have two nodes to form a eurekaServer cluster,
If I set eureka.client.fetch-registry=false and eureka.client.register-with-eureka=true,
then eurekaClient inside the eurekaServer will never fetch registry from another eurekaServer upon startup.

Refer to PeerAwareInstanceRegistryImpl.syncUp:

public int syncUp() {
        // Copy entire entry from neighboring DS node
        int count = 0;

        for (int i = 0; ((i < serverConfig.getRegistrySyncRetries()) && (count == 0)); i++) {
            if (i > 0) {
                try {
                    Thread.sleep(serverConfig.getRegistrySyncRetryWaitMs());
                } catch (InterruptedException e) {
                    logger.warn("Interrupted during registry transfer..");
                    break;
                }
            }
            Applications apps = eurekaClient.getApplications();
            for (Application app : apps.getRegisteredApplications()) {
                for (InstanceInfo instance : app.getInstances()) {
                    try {
                        if (isRegisterable(instance)) {
                            register(instance, instance.getLeaseInfo().getDurationInSecs(), true);
                            count++;
                        }
                    } catch (Throwable t) {
                        logger.error("During DS init copy", t);
                    }
                }
            }
        }
        return count;
    }

Because eureka.client.fetch-registry=false, eurekaClient.getApplications().getRegisteredApplications() here will always be empty, so wait for 5 times here is meaningless. so clientConfig.shouldRegisterWithEureka() should be changed to use clientConfig.shouldFetchRegistry() just as what this PR shows.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #3627 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3627   +/-   ##
=========================================
  Coverage     65.04%   65.04%           
  Complexity     1508     1508           
=========================================
  Files           191      191           
  Lines          7286     7286           
  Branches        871      871           
=========================================
  Hits           4739     4739           
  Misses         2236     2236           
  Partials        311      311
Impacted Files Coverage Δ Complexity Δ
...x/eureka/server/EurekaServerAutoConfiguration.java 85.71% <0%> (ø) 16 <0> (ø) ⬇️

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 455c60c...94aad12. Read the comment docs.

2 similar comments
@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #3627 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3627   +/-   ##
=========================================
  Coverage     65.04%   65.04%           
  Complexity     1508     1508           
=========================================
  Files           191      191           
  Lines          7286     7286           
  Branches        871      871           
=========================================
  Hits           4739     4739           
  Misses         2236     2236           
  Partials        311      311
Impacted Files Coverage Δ Complexity Δ
...x/eureka/server/EurekaServerAutoConfiguration.java 85.71% <0%> (ø) 16 <0> (ø) ⬇️

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 455c60c...94aad12. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #3627 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3627   +/-   ##
=========================================
  Coverage     65.04%   65.04%           
  Complexity     1508     1508           
=========================================
  Files           191      191           
  Lines          7286     7286           
  Branches        871      871           
=========================================
  Hits           4739     4739           
  Misses         2236     2236           
  Partials        311      311
Impacted Files Coverage Δ Complexity Δ
...x/eureka/server/EurekaServerAutoConfiguration.java 85.71% <0%> (ø) 16 <0> (ø) ⬇️

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 455c60c...94aad12. Read the comment docs.

@spencergibb
Copy link
Member

I think that makes sense. Is there a way to test this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants