Skip to content

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

Merged
juststillthinking merged 32 commits into
apache:masterfrom
Fabriceli:fix/consul_support_health_endpoint
May 4, 2023
Merged

fix(consul): support to fetch only health endpoint#9204
juststillthinking merged 32 commits into
apache:masterfrom
Fabriceli:fix/consul_support_health_endpoint

Conversation

@Fabriceli

@Fabriceli Fabriceli commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

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

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

@leslie-tsang
leslie-tsang requested a review from liuxiran April 3, 2023 22:26
@Fabriceli

Copy link
Copy Markdown
Contributor Author

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

@juststillthinking

Copy link
Copy Markdown
Contributor

@Fabriceli The ci has been reruned

@Fabriceli

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 Please take a look at this PR, thanks

@Fabriceli

Copy link
Copy Markdown
Contributor Author

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

@Fabriceli

Copy link
Copy Markdown
Contributor Author

ping @monkeyDluffy6017 @soulbird

@juststillthinking

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

Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua
Comment thread apisix/discovery/consul/init.lua Outdated

@leslie-tsang leslie-tsang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution. :)

Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua Outdated
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
Copy Markdown
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
Copy Markdown
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

@leslie-tsang leslie-tsang May 4, 2023

Copy link
Copy Markdown
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_encode(watch_result, true),

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

Comment thread apisix/discovery/consul/init.lua Outdated
Comment thread apisix/discovery/consul/init.lua
@leslie-tsang
leslie-tsang requested a review from soulbird May 4, 2023 07:54
@juststillthinking
juststillthinking 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