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

Ansible runner cleanup #547

Merged
merged 1 commit into from Jul 28, 2022
Merged

Ansible runner cleanup #547

merged 1 commit into from Jul 28, 2022

Conversation

tinez
Copy link
Member

@tinez tinez commented Jul 21, 2022

  • ansible: Remove unused things
  • AnsibleRunnerClient: make 'mapper' a scoped object

@tinez
Copy link
Member Author

tinez commented Jul 21, 2022

/ost

mnecas
mnecas reviewed Jul 22, 2022
View reviewed changes
...odules/bll/src/main/java/org/ovirt/engine/core/common/utils/ansible/AnsibleRunnerClient.java Outdated
@@ -188,7 +179,6 @@ public int processEvents(String playUuid,
}
}
lastEvent = event;
returnValue.setLastEventId(getLastEventId());
lastEventId++;
}
return lastEvent.isEmpty() ? lastEventId : getLastEventId();
Copy link
Member

Choose a reason for hiding this comment

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

Think that this patch still has the previous issue described in #545.
IMHO the Michals patch looks best #546 but this patch also contains nice refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, while I was going over the code trying to wrap my head around it I noticed some things that are unused. This one is not enough to fix the problem. Now rebased on top of Michal's changes.

@michalskrivanek michalskrivanek mentioned this pull request Jul 22, 2022
Closed
@tinez
ansible: Remove unused things
5dcc52f 
None of the removed things are used:
- we never do anything with neither 'msg' nor 'logFile' arguments
  of 'AnsibleRunnerClient.processEvents'
- the 'AnsibleReturnValue' instance created in 'AnsibleCallback.doPolling'
  is used only locally and doesn't affect the logic
@tinez tinez requested a review from smelamud as a code owner July 27, 2022 16:23
@tinez
Copy link
Member Author

tinez commented Jul 27, 2022

/ost

@michalskrivanek michalskrivanek self-requested a review July 28, 2022 07:58
mwperina
mwperina reviewed Jul 28, 2022
View reviewed changes
...odules/bll/src/main/java/org/ovirt/engine/core/common/utils/ansible/AnsibleRunnerClient.java Outdated
.builder()
.findAndAddModules()
.build()
.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
Copy link
Member

Choose a reason for hiding this comment

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

What's so harmful on having JsonMapper shared between threads? Unless I'm missing something we use that approach a lot in engine, because JsonMapper initialization takes some time ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I thought sharing the mapper between threads might be an issue, but found in the docs: Mapper instances are fully thread-safe provided that ALL configuration of the instance occurs before ANY read or write calls. Thanks!

michalskrivanek
michalskrivanek approved these changes Jul 28, 2022
View reviewed changes
Copy link
Member

@michalskrivanek michalskrivanek left a comment

Choose a reason for hiding this comment

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

I think next step would be synchronize around processEvents, but we would somehow need to handle logger, probably add that again as a parameter

@michalskrivanek
Copy link
Member

/ost

@michalskrivanek michalskrivanek merged commit b3729d2 into oVirt:master Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwperina mwperina mwperina left review comments

@michalskrivanek michalskrivanek michalskrivanek approved these changes

@mnecas mnecas mnecas left review comments

@ahadas ahadas Awaiting requested review from ahadas ahadas is a code owner

@bennyz bennyz Awaiting requested review from bennyz bennyz is a code owner

@emesika emesika Awaiting requested review from emesika emesika is a code owner

@oliel oliel Awaiting requested review from oliel oliel is a code owner

@sgratch sgratch Awaiting requested review from sgratch sgratch is a code owner

@didib didib Awaiting requested review from didib didib is a code owner

@ljelinkova ljelinkova Awaiting requested review from ljelinkova ljelinkova is a code owner

@smelamud smelamud Awaiting requested review from smelamud smelamud is a code owner

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@tinez @michalskrivanek @mwperina @mnecas