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

remove "daemon" setting, add foreground option #453

Merged
merged 4 commits into from
Mar 11, 2021

Conversation

r00t-
Copy link
Contributor

@r00t- r00t- commented Mar 9, 2021

#450 (to also add single-shot mode) is taking a little longer, but propse to merge this to fix #378

@r00t- r00t- requested a review from andig March 9, 2021 18:13
@r00t-
Copy link
Contributor Author

r00t- commented Mar 9, 2021

before that discussion comes up again:
setting daemon=false is NOT a useful mode for running vzlogger.
it will exit the reading thread after getting a reading, but will NOT wait for the logging thread to actually report the data to the api, so it will in most cases be lost.
this has been pointed out long ago, e.g. here: #389 (comment)
a real non-broken single-shot mode is coming up in #450

@r00t- r00t- requested a review from mbehr1 March 9, 2021 19:58
Copy link
Contributor

@mbehr1 mbehr1 left a comment

Choose a reason for hiding this comment

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

code lgtm and makes perfect sense to remove this flaky feature.
As it's removing existing features aka breaking compatibility I
suggest to e.g. increase major version.
From mailing-list I assume lots of people are using the "one-shot" function (which seem to be somewhat working for some config combinations). So we might expect some "flame-wars" :-)

@r00t-
Copy link
Contributor Author

r00t- commented Mar 10, 2021

@mbehr1:
thanks!

complaints from (ab)users might be a valid reason to delay this until #450 is done,
would appreciate if you can provide some assistance there.

this also reminds me again that i should provide the details of the brokenness in the commit message.

need to look into how to bump the version.
i wasn't assuming anybody ever cared about vzlogger version numbers - but i guess the same users you mentioned will.

setting daemon:false was NEVER a useful mode for running vzlogger.
it NOT only disabled unix-"daemonization",
but it made the reading thread exit after getting a single reading,
while it did NOT NOT have vzlogger wait for the logging thread to actually
report the data to the api, so data would have been lost most cases.

to be replaced with one-shot and foreground options.
for now:
always run logging thread,
always daemonize.
this is intentionally not available via the config-file.
partially reverts 1d4972d
@r00t-
Copy link
Contributor Author

r00t- commented Mar 11, 2021

updated commit message and some grammar details.
let's make a separate MR for bumping the version.

@r00t- r00t- merged commit 7787266 into volkszaehler:master Mar 11, 2021
@r00t- r00t- deleted the nodaemon_foreground branch March 11, 2021 02:20
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.

Please add --foreground option
3 participants