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

Add node message support for ACLK new architecture #11514

Merged
merged 4 commits into from
Sep 20, 2021

Conversation

stelfrag
Copy link
Collaborator

Summary

Add initial support for node info message

Component Name

aclk, database

Test Plan

The code is not active yet (it is tested with the new architecture in a separate branch)

@stelfrag stelfrag requested review from underhood and MrZammler and removed request for Ferroin, ktsaou, vkalintiris and thiagoftsm September 15, 2021 12:23
@github-actions github-actions bot added area/build Build system (autotools and cmake). area/database labels Sep 15, 2021
@underhood
Copy link
Contributor

an additional small nit picky comment would be should those files and functions be called sqlite? I know they are called out of your event loop but same time this particular part seems it has nothing to do with database

underhood
underhood previously approved these changes Sep 16, 2021
MrZammler
MrZammler previously approved these changes Sep 17, 2021
@MrZammler MrZammler self-requested a review September 17, 2021 09:28
@MrZammler
Copy link
Contributor

Sorry, noticed some build failures on ubuntu 16.04, will have a quick look...

@stelfrag
Copy link
Collaborator Author

Sorry, noticed some build failures on ubuntu 16.04, will have a quick look...

I see that -- I will take a look as well

@MrZammler
Copy link
Contributor

Sorry, noticed some build failures on ubuntu 16.04, will have a quick look...

I see that -- I will take a look as well

I think if fails because ACLK_NG cannot be built there (lack of protobuf is mentioned in configure), so the compile goes on to build with Legacy. Maybe the #ifdef should be for ACLK_NG ? Cause aclk/aclk_charts_api.c is not built when not in ACLK_NG ?

@MrZammler
Copy link
Contributor

But I also get some (which when handled, proceeds to build ACLK_NG, and the binary links):

aclk/aclk.c: In function ‘ng_aclk_host_state_update’:
aclk/aclk.c:972:47: error: ‘UUID_STR_LEN’ undeclared (first use in this function)
     query->data.node_update.node_id = mallocz(UUID_STR_LEN);

@stelfrag
Copy link
Collaborator Author

But I also get some (which when handled, proceeds to build ACLK_NG, and the binary links):

aclk/aclk.c: In function ‘ng_aclk_host_state_update’:
aclk/aclk.c:972:47: error: ‘UUID_STR_LEN’ undeclared (first use in this function)
     query->data.node_update.node_id = mallocz(UUID_STR_LEN);

Checking ACLK_NG works

Also works as - is when you pass --disable-cloud explicitly

@MrZammler
Copy link
Contributor

MrZammler commented Sep 17, 2021

(lack of protobuf is mentioned in configure)

I think the latest protobuf from their repositories in 16.04 seems to be 2.6.1. In configure we want >3, so checking for PROTOBUF fails, but actually we detect having /usr/bin/protoc. So if you request --use-system-protobuf there, it won't fail, but won't build ACLK_NG either. Maybe we should check for protoc --version > 3 ... ? (Not related to this PR).

@stelfrag stelfrag dismissed stale reviews from MrZammler and underhood via cb78b25 September 17, 2021 11:43
@stelfrag
Copy link
Collaborator Author

Added ACLK_NG check for now

underhood
underhood previously approved these changes Sep 17, 2021
@stelfrag
Copy link
Collaborator Author

Rebased

@stelfrag stelfrag merged commit cb405de into netdata:master Sep 20, 2021
@stelfrag stelfrag deleted the ng_node branch September 20, 2021 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/database
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants