-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: set default maxTimeMS
when creating an index; unhandled exc on setup()
will exit kytosd
#414
Conversation
parametrized maxTimeMS with env var MONGO_IDX_TIMEOUTMS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left a comment regarding print
(not sure it was on purpose). Once the end-to-end tests finish, hopefully with good results, you have my approval.
except Exception as exc: | ||
exc_fmt = traceback.format_exc(chain=True) | ||
message = f"Kytos couldn't start because of {str(exc)} {exc_fmt}" | ||
print(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to double check if this should be print
instead of self.log.critical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @italovalcy, yes, initially it was self.log.exception
(relatively similar to self.log.critical
), but I had to revert it on f4432f9. In summary, at this point here, since we're using QueueListener
by default and it's threaded, it might end up not ready yet, so even a self.log.listener.stop()
doesn't guarantee that it'd print out correctly (so it might lose the message before emiting the event or processing it). So, print
ensure it's deterministic here, similarly to other parts of the code were either the log isn't ready or setup yet https://github.com/kytos-ng/kytos/blob/master/kytos/core/controller.py#L175
# pylint: disable=broad-except | ||
try: | ||
if self.options.database: | ||
self.db_conn_or_core_shutdown() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's room to also allow both db_conn_or_core_shutdown
and init_apm_or_core_shutdown
to reraise as mentioned on #415, but I'll only address in the feature since it's just a chore, and that way it also minimizes the chunks of code that need to be backported.
Thanks for your review, Italo. e2e tests have passed with this branch. I'll merge it soon. |
Closes #413 (check out the last comments there for more information)
Partly addresses #314
Summary
See updated changelog file
Local Tests
OperationFailure
(equivalent to index creation op error), confirmed the retries also worked as expected, and confirmed the traceback was included (this will generate some extra information, but when this happens, sometimes it can be an actual bug, so the traceback will be needed for reporting a bug):flow_manager
, noticed that the traeback was chained correctly just so you can trace it completely:End-to-End Tests
I'll dispatch an exec on GitLab, I'll post the results here later: