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

Add Stacktrace when DagFileProcessorManager gets killed #10681

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

msumit
Copy link
Contributor

@msumit msumit commented Sep 1, 2020

Scheduler - DagFileProcessorManager is the process that often gets hung due to various reasons. Found that having access to the stacktrace would be very helpful in debugging the root cause.

Should be helpful in resolving issue #7935

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Sep 1, 2020
Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Given we still officially support Python 3.5, maybe not the best time yet to introduce f-string (which is only supported since 3.6)?

@potiuk
Copy link
Member

potiuk commented Sep 2, 2020

Given we still officially support Python 3.5, maybe not the best time yet to introduce f-string (which is only supported since 3.6)?

But we are already using a lot of f-strings. 3.5 is only supported in 1.10.* line. In master we are 3.6+

@XD-DENG
Copy link
Member

XD-DENG commented Sep 2, 2020

Given we still officially support Python 3.5, maybe not the best time yet to introduce f-string (which is only supported since 3.6)?

But we are already using a lot of f-strings. 3.5 is only supported in 1.10.* line. In master we are 3.6+

Cool, makes sense to me.

@msumit
Copy link
Contributor Author

msumit commented Sep 3, 2020

@potiuk @XD-DENG can you guys approve it, if looks fine?

Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

👍

@potiuk
Copy link
Member

potiuk commented Sep 3, 2020

Ah. And now @XD-DENG is going to kill me :) ...

I commented the "we are past 3.5" case, but when I looked at this closely .... I think you should not use the f-string in fact.

For a completely different reason though. It's OK to use f-strings in general - when it comes to logic or info messages, but in case of debug statements - it's not a good idea because of performance reasons. Debug statements are usually not printed in "production" environment. But if you use f-string or .format(), those interpolations are executed regardless of the setting of log level.

So for debug statements you should use this form:

    log.debug("Disposing DB connection pool (PID %s)", os.getpid())

As this will only run string interpolation when it is about to be printed.

Copy link
Member

@XD-DENG XD-DENG left a comment

Choose a reason for hiding this comment

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

Change my review status based on latest comment from @potiuk

@XD-DENG XD-DENG self-requested a review September 3, 2020 11:26
@XD-DENG
Copy link
Member

XD-DENG commented Sep 3, 2020

Ah. And now @XD-DENG is going to kill me :) ...

I commented the "we are past 3.5" case, but when I looked at this closely .... I think you should not use the f-string in fact.

For a completely different reason though. It's OK to use f-strings in general - when it comes to logic or info messages, but in case of debug statements - it's not a good idea because of performance reasons. Debug statements are usually not printed in "production" environment. But if you use f-string or .format(), those interpolations are executed regardless of the setting of log level.

So for debug statements you should use this form:

    log.debug("Disposing DB connection pool (PID %s)", os.getpid())

As this will only run string interpolation when it is about to be printed.

Definitely would love instead of kill you😄😄 thanks for taking the second look and the update @potiuk !

@msumit msumit merged commit faaf179 into apache:master Sep 3, 2020
@kaxil
Copy link
Member

kaxil commented Sep 3, 2020

Nice one @msumit

@msumit msumit deleted the add_stacktrace branch September 3, 2020 18:32
msumit added a commit to twitter-forks/airflow that referenced this pull request Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants