Skip to content

Change to use shouldFetchRegistry.#3627

Open
aftersss wants to merge 1 commit intospring-cloud:mainfrom
aftersss:master-fix1
Open

Change to use shouldFetchRegistry.#3627
aftersss wants to merge 1 commit intospring-cloud:mainfrom
aftersss:master-fix1

Conversation

@aftersss
Copy link
Copy Markdown

@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
Copy Markdown

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

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

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
Copy Markdown
Member

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

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.04%. Comparing base (455c60c) to head (94aad12).
⚠️ Report is 1062 commits behind head on main.

Files with missing lines Patch % Lines
...x/eureka/server/EurekaServerAutoConfiguration.java 0.00% 0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #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           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

4 participants