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.

Sign up for GitHub

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

Jump to bottom

Rework comment handling #420

Merged
merged 6 commits into from
Jun 22, 2023
Merged

Rework comment handling #420

merged 6 commits into from
Jun 22, 2023

Conversation

sni
Copy link
Contributor

@sni sni commented Mar 1, 2023

rework comment handling

This PR reworks the comment handling to improve performance when having lots of comments.

  • it add a prev pointer to the comments list so we don't have to iterate over the complete list when removing an item
  • it replaces the hash lookup to quickly access comments for specific hosts/services in favor of a new host/service attribute which holds links to the comments
  • it avoids duplicate host/service lookups for DEL_ALL_HOST_COMMENTS/DEL_ALL_SVC_COMMENTS command
  • it removes hash functions which are now unused

In general, it tries to adopt the downtime handling over to comments.

This PR is based on and includes #375.

Since the internal object structure changes, all NEB modules have to be rebuild. (We therefor increase CURRENT_NEB_API_VERSION to 6)

Signed-off-by: Sven Nierlein [email protected]

nook24 and others added 2 commits March 1, 2023 10:59
@nook24 @sni
- Use hasmap to find comment data
e5f8474 
Signed-off-by: nook24 <[email protected]>
@sni
rework comment handling
1d5dfa0 
This PR reworks the comment handling to improve performance when having lots of comments.

  - it add a prev pointer to the comments list so we don't have to iterate over the complete list when removing an item
  - it replaces the hash lookup to quickly access comments for specific hosts/services in favor of a new host/service attribute which holds links to the comments
  - it avoids duplicate host/service lookups for DEL_ALL_HOST_COMMENTS/DEL_ALL_SVC_COMMENTS command
  - it removes hash functions which are now unused

In general, it tries to adopt the downtime handling over to comments.

This PR is based on naemon#375.

Since the internal object structure changes, all NEB modules have to be rebuild. (We therefor increase CURRENT_NEB_API_VERSION to 6)

Signed-off-by: Sven Nierlein <[email protected]>
@sni sni mentioned this pull request Mar 1, 2023
Closed
sni and others added 3 commits March 1, 2023 13:02
@sni
add timeing point for number of downtimes/comments
c18fa90 
Signed-off-by: Sven Nierlein <[email protected]>
@sni
fix typo in get_new_comment_id
370590c 
swapped id and type parameter. Also start ids at 1 like before.
@sni
fix downtime test
f45c2e1 
comment ids start at 1.
@jdumalaonITRS
Copy link

Hi,
Not sure if I missed something but doesn't it look like we need the comment_list even less?
We can still have the host/service attribute which holds links to the comments and have a hashtable.

I am just not 100% sure what needs to happen for the sorting code for comment_list though. Does anyone know why we need to sort?

@sni
Copy link
Contributor Author

sni commented Mar 9, 2023

I assumed the comment_list is required for livestatus, but i did a quick search and did not find any reference. It seems like comments are soley add/removed from broker callbacks.
I can try to remove it completely. And if it's removed, we don't have to order it.

@sni
remove comment_list
613a5b4 
it is not required anymore since we store comments in a hash table now.

Signed-off-by: Sven Nierlein <[email protected]>
@sni
Copy link
Contributor Author

sni commented Mar 20, 2023

Alright, i completely removed the comments_list now.

@jdumalaonITRS
Copy link

Nice thanks for this.
The code looks good. Let me try some manual tests for this. I'll try to feedback by next week.

@sni sni mentioned this pull request Mar 29, 2023
Open
jdumalaonITRS
jdumalaonITRS approved these changes Apr 26, 2023
View reviewed changes
Copy link

@jdumalaonITRS jdumalaonITRS left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

nook24
nook24 reviewed Apr 26, 2023
View reviewed changes
src/naemon/comments.c
host *temp_host = find_host(this_comment->host_name);
remove_object_from_objectlist(&temp_host->comments_list, this_comment);
}
else if (type == SERVICE_COMMENT) {
Copy link
Member

Choose a reason for hiding this comment

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

Personally i would prefer a simple else statement here

@nook24
Copy link
Member

nook24 commented Apr 26, 2023

I have tested this with 400k comments, works as promised.

[0.0079 (+0.0000)] Initialized retention data
[0.0079 (+0.0000)] Reading initial state information
[2.4028 (+2.3948)] Read initial state information
[2.4029 (+0.0002)] Restored 0 downtimes
[2.4029 (+0.0000)] Restored 400000 comments
[2.4030 (+0.0000)] Initializing performance data

@sni sni merged commit 12de0b0 into naemon:master Jun 22, 2023
@sni sni deleted the rework_comment_handling branch February 9, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdumalaonITRS jdumalaonITRS jdumalaonITRS approved these changes

@nook24 nook24 nook24 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sni @jdumalaonITRS @nook24