Skip to content

Virtualize ServerApplication::handlePidFile()#4223

Merged
aleks-f merged 5 commits intodevelfrom
4181-add-optionsetreplaceoption
Nov 21, 2023
Merged

Virtualize ServerApplication::handlePidFile()#4223
aleks-f merged 5 commits intodevelfrom
4181-add-optionsetreplaceoption

Conversation

@pavledragisic
Copy link
Copy Markdown
Member

No description provided.

@pavledragisic pavledragisic self-assigned this Oct 25, 2023
@pavledragisic pavledragisic linked an issue Oct 25, 2023 that may be closed by this pull request
@pavledragisic pavledragisic requested a review from aleks-f October 25, 2023 14:11
@obiltschnig
Copy link
Copy Markdown
Member

Why doesn't replaceOption() take an Option parameter, like addOption()? Seems inconsistent.

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

Why doesn't replaceOption() take an Option parameter, like addOption()? Seems inconsistent.

@pavledragisic change it, but it also requires changes on our side (devs, dispatcher and s7server) so either align it on our side or create an issue there

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

@obiltschnig now that I think about it, I'd keep both versions. The string version is convenient because there's no need to create Option on the call side

@obiltschnig
Copy link
Copy Markdown
Member

Then at least also add an addOption() overload, please, so that the API stays consistent.

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

Then at least also add an addOption() overload, please, so that the API stays consistent.

alright, @pavledragisic do it like that

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

@obiltschnig after a bit more thinking, how about this instead:

template <typename T>
void OptionSet::replaceHandler(const std::string& opt, T* pObj, Callback<T> func)

That is really what we need - to tell the Application that we will handle this option and replacing the whole Option is the proverbial cannon to kill a mosquito :)

@obiltschnig
Copy link
Copy Markdown
Member

If that's all you need, then how about just making handlePidFile(), handleDaemon(), etc. virtual?

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

If that's all you need, then how about just making handlePidFile(), handleDaemon(), etc. virtual?

virtualizing will work; at this time, I would limit it to handlePidFile only and I would also like to move it out of #ifdef FAMILY_UNIX, so that it is available on all platforms

@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Oct 25, 2023

If that's all you need, then how about just making handlePidFile(), handleDaemon(), etc. virtual?

virtualizing will work; at this time, I would limit it to handlePidFile only and I would also like to move it out of #ifdef FAMILY_UNIX, so that it is available on all platforms

@pavledragisic do it like this ^

void handlePidFile(const std::string& name, const std::string& value);

discard current changes in this branch, move this function out of #ifdef, and make it virtual. We then have to override it in our processes that require strict PID creation timing

@aleks-f aleks-f changed the title Add OptionSet::replaceOption() Virtualize ServerApplication::handlePidFile() Oct 26, 2023
@aleks-f aleks-f merged commit f30d759 into devel Nov 21, 2023
@aleks-f aleks-f deleted the 4181-add-optionsetreplaceoption branch November 21, 2023 05:36
@aleks-f aleks-f added this to the Release 1.13.0 milestone Nov 21, 2023
aleks-f added a commit that referenced this pull request Nov 23, 2023
* feat(OptionSet): Add replaceOption() #4181

* revert changes #4181

* feat: make ServerApplication::handlePidFile virtual #4181

* move handlePidFile() out of ifdef #4181

* fix(ServerApplication): move handlePidFile() out of all ifdefs #4181

---------

Co-authored-by: Pavle <pavle@debian-gnu-linux-11.localdomain>
Co-authored-by: Aleksandar Fabijanic <aleks-f@users.noreply.github.com>
aleks-f added a commit that referenced this pull request Nov 27, 2023
* feat(OptionSet): Add replaceOption() #4181

* revert changes #4181

* feat: make ServerApplication::handlePidFile virtual #4181

* move handlePidFile() out of ifdef #4181

* fix(ServerApplication): move handlePidFile() out of all ifdefs #4181

---------

Co-authored-by: Pavle <pavle@debian-gnu-linux-11.localdomain>
Co-authored-by: Aleksandar Fabijanic <aleks-f@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Virtualize ServerApplication::handlePidFile()

3 participants