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

Requires api_helper_indexes plugin to start node #1889

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@abitmore
Copy link
Member

commented Aug 8, 2019

#1836 introduced a wlog in constructor of database_api_impl, this PR changes it to dlog because wlog will cause disk space issue on API nodes (see #1799 and #1798).

@abitmore abitmore added this to the 3.3.0 - Feature Release milestone Aug 8, 2019

@abitmore abitmore added this to In development in Feature Release (3.3.0) via automation Aug 8, 2019

@abitmore abitmore requested a review from pmconrad Aug 8, 2019

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

This is a warning on purpose. API nodes should enable the plugin because otherwise clients will receive incomplete information. Changing the log level to debug means the node owner won't notice his configuration problem.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Perhaps we add an option so the admins who really want to disable the plugin can disable the logging?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Hm, it feels wrong to add an option for a single log message.
IMO node owners who want to run API nodes without that plugin can post-process the logfiles to remove the message if it annoys them.
I can't really imagine a use-case where that makes much sense though. An API node is either public and should enable the plugin, or it's private and sees little API traffic and therefore only few of these messages. (Or maybe it's a private power user with a lot of API traffic - but I'd expect a power user to be able to tweak logging the way he wants it.)

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

I think it's better to crash the node on start if we want to force the user to enable that plugin, than crash the node due to disk space issue after it's running for a few days. Better bother the admins once than twice.

Then, it would be a good idea to add an option to avoid the crash.

Requires api_helper_indexes plugin to start node
or start with --ignore-api-helper-indexes-warning.
Don't silently write warning messages to P2P log.

@abitmore abitmore force-pushed the abitmore-patch-1 branch from abd014e to fa2248f Aug 13, 2019

@abitmore abitmore changed the title Use debug logging level in database_api_impl Requires api_helper_indexes plugin to start node Aug 13, 2019

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Force pushed. @pmconrad please review.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

IMO this option is annoying for non-API node admins. Please move the check into application::reset_websocket... so the option is only required for nodes that actually provide API endpoints.

@abitmore

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

application::reset_websocket() is buried too deep in the call flow, so I added the checking in main(), the outcome should be the same.

@pmconrad
Copy link
Contributor

left a comment

Looks good, thanks!

Feature Release (3.3.0) automation moved this from In development to In testing Aug 14, 2019

@abitmore abitmore merged commit f9eeb76 into develop Aug 14, 2019

3 checks passed

ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature Release (3.3.0) automation moved this from In testing to Done Aug 14, 2019

@abitmore abitmore deleted the abitmore-patch-1 branch Aug 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.