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

WIP: Change the order of the API runtime object sync on reconnect #7786

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

htriem
Copy link
Contributor

@htriem htriem commented Jan 27, 2020

Edit (@Al2Klimov)

Rationale: see #7786 (comment)

closes #10000

@htriem htriem requested a review from dnsmichi January 27, 2020 15:00
@htriem htriem added area/configuration DSL, parser, compiler, error handling core/quality Improve code, libraries, algorithms, inline docs labels Jan 27, 2020
@dnsmichi dnsmichi changed the title WIP: Change the order of the object sync WIP: Change the order of the API runtime object sync on reconnect Jan 27, 2020
@dnsmichi dnsmichi added the bug Something isn't working label Jan 27, 2020
@dnsmichi dnsmichi added the area/api REST API label Feb 7, 2020
@Al2Klimov Al2Klimov marked this pull request as draft June 29, 2020 10:08
@Al2Klimov Al2Klimov added this to the 2.13.0 milestone Nov 23, 2020
@julianbrost julianbrost removed this from the 2.13.0 milestone May 31, 2021
@julianbrost
Copy link
Contributor

@htriem Please either state the purpose of the PR or close it if you don't consider it important (anymore).

@julianbrost julianbrost removed the request for review from dnsmichi September 9, 2021 10:16
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Concept looks good, rebase it against master.

@julianbrost
Copy link
Contributor

What's the purpose of the change? Does it fix a bug? Is it a performance improvement?

@Al2Klimov
Copy link
Member

If there's a bug, it fixes it at the cost of performance. It re-orders runtime updates for pretty much the same reasons as we order objects while loading config initially. Once Henrik rebased this and there's something to test I maybe find a way to provoke the bug if any.

@julianbrost
Copy link
Contributor

I don't really understand. There must be a reason why this PR was created and you must have had a reason why you consider it a good concept.

@Al2Klimov
Copy link
Member

Damn, the reason I had in mind has been removed by 28c29c1. For now feel free to consider it a just-to-be-sure.

@htriem htriem force-pushed the bugfix/api-runtime-object-sync-order branch from 1faa8a1 to e627dcc Compare January 24, 2023 11:11
@cla-bot cla-bot bot added the cla/signed label Jan 24, 2023
@htriem
Copy link
Contributor Author

htriem commented Jan 24, 2023

Rebased it on top of the master.

@julianbrost
Copy link
Contributor

For now feel free to consider it a just-to-be-sure.

So the purpose is to increase the robustness of the object config sync?

I've looked at the change, and one situation where I can imagine this might help would be that while a child node is offline, a host and service are created for example. If the child node reconnect, could it happen without that change that the service is sent first and then fails to load? But haven't tried any of this, just wild guessing from looking at the code.

But really, if there's a proposed change, there should be a description with a motivation for the change. It shouldn't need someone else looking at the code and guessing the purpose.

@Al2Klimov
Copy link
Member

That was also my guess, but I wanted to first test it once there is something to test. Apropos...

Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Pipelines fail

@julianbrost
Copy link
Contributor

Wouldn't you test if something is broken on master or a release? So that can be done before anyone puts effort into updating an old PR.

@Al2Klimov
Copy link
Member

Damn, the reason I had in mind has been removed by 28c29c1

Hence:

@htriem Please either state the purpose of the PR

ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Sep 12, 2023
ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 6, 2023
@ymartin-ovh ymartin-ovh mentioned this pull request Oct 6, 2023
ymartin-ovh pushed a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 6, 2023
ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 6, 2023
ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 9, 2023
ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 9, 2023
ymartin-ovh added a commit to ymartin-ovh/icinga2 that referenced this pull request Oct 9, 2023
@Al2Klimov
Copy link
Member

@julianbrost OK, now I have enough information to give you an answer:

#9752 shouldn't happen, right? But it happens – a comment's host is missing. I thought, maybe the host is only missing yet. Maybe it should just get send over the wire before the comment itself, hence this PR. But #9873 (comment) says no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api REST API area/configuration DSL, parser, compiler, error handling bug Something isn't working cla/signed core/quality Improve code, libraries, algorithms, inline docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants