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

fix(consul): support to fetch only health endpoint #9204

Merged

Conversation

Fabriceli
Copy link
Contributor

@Fabriceli Fabriceli commented Mar 30, 2023

Description

Thank you for your previous work @dyrnq

Create TWO threads to watch the catalog services and health services:

flowchart TD
		subgraph Thread 1: watch catalog
		A[Start] --> catalog{Catalog any change?}
    catalog -->|Yes| cindex[return catalog index]
    catalog ---->|No| cblock[block query until catalog any change]
    cblock --> E[End]
    cindex --> E
    end
    
    subgraph Thread 2: watch health
    A2[Start] --> health2{health any change?}
    health2 -->|Yes| hindex[return health index]
    health2 ---->|No| hblock[block query until health any change]
    hblock --> E2[End]
    hindex --> E2
    end
    
    subgraph Wait any thread
    A3[Start] --> spawn[Spawn TWO threads]
    spawn --> wait[wait any Thread]
    wait --> catalog2[Query all services]
    catalog2 --> H[Query health nodes]
    H --> update[Update all services]
    update --> e[End]
    end
Loading

Fixes # (issue)
Reference: #8856, #8928

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@Fabriceli
Copy link
Contributor Author

@tokers @spacewander @tzssangglass Please take a look at this PR, thanks

@Fabriceli
Copy link
Contributor Author

@liuxiran Could you return CI? some other test error, which is not I add

@monkeyDluffy6017
Copy link
Contributor

@Fabriceli The ci has been reruned

@Fabriceli
Copy link
Contributor Author

@monkeyDluffy6017 Please take a look at this PR, thanks

@Fabriceli
Copy link
Contributor Author

@monkeyDluffy6017 The CI is not very stable,could you re-run the CI?

@Fabriceli
Copy link
Contributor Author

ping @monkeyDluffy6017 @soulbird

@monkeyDluffy6017
Copy link
Contributor

Hi @Fabriceli, thank you for all your hard work!
I'm working on this pr, it will take me some time to figure it out.

Copy link
Member

@leslie-tsang leslie-tsang left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. :)

apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
if watch_error_info then
log.error("connect consul: ", consul_server.consul_server_url,
" by sub url: ", consul_server.consul_watch_health_url,
", got watch result: ", json_delay_encode(watch_result, true),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
", got watch result: ", json_delay_encode(watch_result, true),
", got watch result: ", json_delay_encode(watch_result, true),

No need for json_delay_encode as the log level is error by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, done

Copy link
Member

@leslie-tsang leslie-tsang May 4, 2023

Choose a reason for hiding this comment

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

Suggested change
", got watch result: ", json_delay_encode(watch_result, true),
", got watch result: ", json_encode(watch_result, true),

Sry for the mistake, we can use json_encode instead of json_delay_encode here. WDYT ?

apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
apisix/discovery/consul/init.lua Outdated Show resolved Hide resolved
@leslie-tsang leslie-tsang requested a review from soulbird May 4, 2023 07:54
@monkeyDluffy6017 monkeyDluffy6017 merged commit 2cd71ce into apache:master May 4, 2023
@Fabriceli Fabriceli deleted the fix/consul_support_health_endpoint branch May 4, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants