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

Set default logger in logging Mixin #20355

Merged
merged 1 commit into from Dec 28, 2021
Merged

Conversation

kazanzhy
Copy link
Contributor

As was written in this file "LoggingMixin should have a default _log field."
So I think there is a time to do this. I will be very glad to any suggestions.

Motivation:
Now overriding the method __getattr__ in every child of this mixin causes recursion and as a result RecursionError.

potiuk
potiuk previously approved these changes Dec 16, 2021
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Dec 16, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Dec 16, 2021

Seems like this one detected an interestig error - some of the classes using Logging Mixin do not call super.init() - and cause our tests to fail

@kazanzhy
Copy link
Contributor Author

kazanzhy commented Dec 17, 2021

I just added one more check and seems ok
This check ok with my future purposes and work almost as try ... except AttributeError

@potiuk
Copy link
Member

potiuk commented Dec 19, 2021

I need a second committer approval on that one

@kazanzhy
Copy link
Contributor Author

I found the commit when this comment was added.
66b2a47#diff-af25dd96233a97ace811c614fa7d5bd059cbbc1571e421f6675e16f6290814c5

@kazanzhy
Copy link
Contributor Author

Hi @kaxil @ashb

I found the following message in the ligging_mixin code that was added by this commit 66b2a47
# FIXME: LoggingMixin should have a default _log field.

Could I ask you this PR if my fix is correct?

# FIXME: LoggingMixin should have a default _log field.
return self._log # type: ignore
except AttributeError:
if not hasattr(self, '_log') or self._log is None:
Copy link
Member

Choose a reason for hiding this comment

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

Why is the hasattr() check needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the classes that inherit from LoggingMixin don't call super().__init__().
I counted above 5 of them.
For that cases hasattr is needed.
I assume that or self._log is None could be deleted.

Copy link
Member

@potiuk potiuk Dec 23, 2021

Choose a reason for hiding this comment

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

Yep. The logging Mixin is not consistently used. I think it's safer to leave it here I think as it's rather likely there will be other cases where init() is not called for the Mixin.

Copy link
Member

Choose a reason for hiding this comment

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

How about making _log a class variable instead?

class LoggingMixin:
    _log: Optional[logging.Logger] = None

    def __init__(self, context=None):
        self._set_context(context)

    @property
    def log(self) -> Logger:
        """Returns a logger."""
        if self._log is None:
            self._log = ...
        return self._log

I think this should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

The advantage of this solution will be the absence of usage hasattr function and all child classes will have _log attribute.

Firstly I thought that if the child class doesn't use .log then _log attribute just will allocate memory useless. But then I realized that I don't see a reason to inherit from LoggingMixin and not use .log.

Copy link
Member

@uranusjr uranusjr Dec 24, 2021

Choose a reason for hiding this comment

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

Class attributes are not allocated for each subclass, only once for the parent class that defines it, so subclasses would have no additional allocations to choose waste, even if they do inherit LoggingMixin without using log.

@potiuk potiuk merged commit 2cd4ed0 into apache:main Dec 28, 2021
@kazanzhy kazanzhy deleted the fix_logging_mixin branch January 1, 2022 13:10
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants