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

Move ElasticsearchTaskHandler to the provider package #9623

Closed
wants to merge 4 commits into from
Closed

Move ElasticsearchTaskHandler to the provider package #9623

wants to merge 4 commits into from

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Jul 2, 2020


This PR fixes one of the issues listed in #9386

The ElasticsearchTaskHandler class from airflow.utils.log.es_task_handler was moved to
airflow.providers.elasticsearch.log.es_task_handler.

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@turbaszek
Copy link
Member

@ephraimbuddy can you please take a look at the CI errors?

@ephraimbuddy
Copy link
Contributor Author

Hi @turbaszek , what could be the cause of the backport packages CI build error? I can't seem to figure it out

@potiuk
Copy link
Member

potiuk commented Jul 2, 2020

Look at the logs @ephraimbuddy -> the "ExternalLoggingMixin" is a bit strange as I cannot see it in the logging_mixin. Where is it from?


Traceback (most recent call last):
File "/import_all_provider_classes.py", line 61, in import_all_provider_classes
_module = importlib.import_module(module_name)
File "/usr/local/lib/python3.6/importlib/init.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "", line 994, in _gcd_import
File "", line 971, in _find_and_load
File "", line 955, in _find_and_load_unlocked
File "", line 665, in _load_unlocked
File "", line 678, in exec_module
File "", line 219, in _call_with_frames_removed
File "/usr/local/lib/python3.6/site-packages/airflow/providers/elasticsearch/log/es_task_handler.py", line 34, in
from airflow.utils.log.logging_mixin import ExternalLoggingMixin, LoggingMixin
ImportError: cannot import name 'ExternalLoggingMixin'


ERROR ENCOUNTERED!

@potiuk
Copy link
Member

potiuk commented Jul 2, 2020

We can't use ExtenralLoggingMixin in Airflow 1.10 providers because it only appeared .... TODAY in master

@potiuk
Copy link
Member

potiuk commented Jul 2, 2020

If we want to support ExternalLoggingMixin we should also bacport it to 1.10 but this might be a bit more complex - using Bowler refactoring (@turbaszek WDYT?)

@ephraimbuddy
Copy link
Contributor Author

@potiuk Thanks!

@mik-laj
Copy link
Member

mik-laj commented Jul 2, 2020

@potiuk This will cherry-pick changes to the Javascript code. If you are willing, we can do it. For now, we should delete references to this class in backport packages. This will allow these classes to be used without this one new feature.

@ephraimbuddy
Copy link
Contributor Author

These changes have been move to #9604

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants