Skip to content

[Improvement] Can't get zookeeper server list in the monitor web page.#5659

Closed
echohlne wants to merge 1 commit intoapache:devfrom
echohlne:zookeeper_monitor_empty
Closed

[Improvement] Can't get zookeeper server list in the monitor web page.#5659
echohlne wants to merge 1 commit intoapache:devfrom
echohlne:zookeeper_monitor_empty

Conversation

@echohlne
Copy link
Contributor

Purpose of the pull request

Make sure the server info of zookeeper can be sent to the front.
git clone the latest branch of dev try to run the entire project in windows os, the rest api would always returns null:
Request URL: http://localhost:8888/dolphinscheduler/monitor/zookeeper/list?_t=0.6361127216024349
Response: {"code":0,"msg":"success","data":null}

Brief change log

an enancement to the pr #5562, try to add an Unmodified Map to store the registry. properties in RegistryCenter to make sure we can get registry.servers in RegistryMonitor.

Verify this pull request

Manually verified the change by testing locally

provide the ablitity to get zookeeper serverlist
@echohlne
Copy link
Contributor Author

@CalvinKirs , do you have any idea about this pr? I'm not sure whether it's the best way to solve the problem.

@CalvinKirs
Copy link
Member

@CalvinKirs , do you have any idea about this pr? I'm not sure whether it's the best way to solve the problem.

Some of the information obtained in the api belongs to the unique information of zk. I am not sure whether there is a good abstraction method. This is related to whether the specific plug-in is provided.

Yes, at present, we query two kinds of data from zookeeper in api.
One is the information of Master and Worker, these data can be provided by all kinds of Registry.
The other is zookeeper's own information, these data can be considered as registry's own metrics.

@CalvinKirs
Copy link
Member

image

@CalvinKirs CalvinKirs added the discussion discussion label Jun 17, 2021
@sonarqubecloud
Copy link

@CalvinKirs
Copy link
Member

@CalvinKirs , do you have any idea about this pr? I'm not sure whether it's the best way to solve the problem.

Some of the information obtained in the api belongs to the unique information of zk. I am not sure whether there is a good abstraction method. This is related to whether the specific plug-in is provided.

Yes, at present, we query two kinds of data from zookeeper in api.
One is the information of Master and Worker, these data can be provided by all kinds of Registry.
The other is zookeeper's own information, these data can be considered as registry's own metrics.

This will require each plugin to be able to provide such metrics information.

@echohlne
Copy link
Contributor Author

echohlne commented Jun 17, 2021

@CalvinKirs Thanks for your reply.
I don't know much about the background of this refractor, but for now, it seems ZK-related information really shouldn't be treated as properties of abstract methods.

@CalvinKirs
Copy link
Member

@CalvinKirs Thanks for your reply.
I don't know much about the background of this refractor, but for now, it seems ZK-related information really shouldn't be treated as properties of abstract methods.
How do you feel about adding a new class of monitoring specifically for ZK, such as adding a ZKRegistryMonitor class in addition to the RegistryMonitor for the ZooKeePerRegistry?
I can't think of a better way...

I'm considering whether I need to remove the monitoring of the registry, which in my opinion is not something that DS needs to manage. WDYT?

@echohlne
Copy link
Contributor Author

echohlne commented Jun 17, 2021

@CalvinKirs Thanks for your reply.
I don't know much about the background of this refractor, but for now, it seems ZK-related information really shouldn't be treated as properties of abstract methods.

I'm considering whether I need to remove the monitoring of the registry, which in my opinion is not something that DS needs to manage. WDYT?

+1, zookeeper's own information can be treated as zookeeper registry's own metrics, there should be a better way to do the monitoring work.

@CalvinKirs
Copy link
Member

Thanks for your contribution, we discussed related issues in the email (https://lists.apache.org/thread.html/r7d92584ad737293835cf4f2023a8a9eea32d7dd5b5c379e4297e66d4%40%3Cdev.dolphinscheduler.apache.org%3E),
so zookeeper monitoring will be removed. For the more important part of the change, I suggest you issue or e-mail discussion first, and thank you again for your contribution.

@CalvinKirs CalvinKirs closed this Jun 29, 2021
@echohlne echohlne deleted the zookeeper_monitor_empty branch June 30, 2021 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants