Adding ElasticserachPythonHook - ES Hook With The Python Client #24895

alexkruc · 2022-07-07T08:53:52Z

Adding a new hook for work with Elasticsearch, using the official Elasticsearch Python client.
Currently, I've implemented only one function (search), but if any more will be needed, we can add even more.

This hook was also tested with a local ES docker image, to make sure that everything works :)

closes: #21929

eladkal · 2022-07-07T09:01:36Z

airflow/providers/elasticsearch/hooks/elasticsearch_python.py

@@ -0,0 +1,56 @@
+#


So we have two ways to interact with elastic one is Python based with the SDK and one is SQL based by using sql query syntax.

i think we should rename this file as python.py or something similar?
and the current ElasticsearchHook in hooks/elasticsearch.py

airflow/airflow/providers/elasticsearch/hooks/elasticsearch.py

Line 27 in 7a9c03c

class ElasticsearchHook(DbApiHook):

should be hooks/sql.py with ElasticsearchSqlHook

Alternatively, we can have both classes ElasticsearchSqlHook + ElasticsearchPythonHook in the same hooks/elasticsearch.py ?

I will add them to the same files, as I think having a file named python.py is a bit confusing..
I don't know how it will play with the documentation files, but I'll try to make it work :)

eladkal · 2022-07-07T10:00:35Z

tests/providers/elasticsearch/hooks/test_elasticsearch.py

 from airflow.models import Connection
-from airflow.providers.elasticsearch.hooks.elasticsearch import ElasticsearchHook
+from airflow.providers.elasticsearch.hooks.elasticsearch import ElasticsearchHook, ElasticsearchPythonHook


Please use the new class name on the import

Done- the old hook tests are now using the new name.
I've also added a test that checks the generation of a DepracationWarning for ElasticsearchHook :)

eladkal · 2022-07-07T10:01:58Z

airflow/providers/elasticsearch/hooks/elasticsearch.py

+    :param hosts: list: A list of a single or many Elasticsearch instances. Example: ["http://localhost:9200"]
+    :param es_conn_args: dict: Additional arguments you might need to enter to connect to Elasticsearch.
+                                Example: {"ca_cert":"/path/to/http_ca.crt", "basic_auth": "(user, pass)"}


It looks like we need also a connection.rst to explain this additional settings for the connection

I've added a new document for both hooks under hooks/elasticsearch.rst

eladkal · 2022-07-07T10:02:27Z

tests/system/providers/elasticsearch/example_elasticsearch_python_hook.py

@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Rename the file example_elasticsearch_hook.py ?

I've merged the new example into the old examples file :)
Now it should give the example to both hooks

potiuk · 2022-07-07T23:39:30Z

Docs now :(

alexkruc · 2022-07-08T05:31:55Z

@potiuk Is it possible to get your assistance on the failing tests?
I have 2 failing tests:

The test TestElasticsearchProviderProjectStructure is failing. From what I understand, it looks for all the available hooks, and looks that they have a matching example in the examples folder actual test.
The thing is, that it's failing on the ElasticsearchHook which I deprecated (because I renamed it) in this PR. I don't want to add an example for a deprecated hook, because it kinda beets the purpose.. Do you have any tips?
The build docs test is failing, on the new documentation that I've added - hooks/elasticsearch.rst.rst:37:non-whitespace stripped by dedent. I compared the dedent clause with several other rst files, and it looked fine to me.. Also, the pre-commit hooks are passing on this file without any changes/failures.. Is there any possibility you can help me with this?

Sorry for the trouble :( I'm just kinda stuck and need an external opinion 🙏

potiuk · 2022-07-08T09:01:11Z

Rebase (I just did).
Let's see.

potiuk · 2022-07-08T09:03:02Z

The test TestElasticsearchProviderProjectStructure is failing. From what I understand, it looks for all the available hooks, and looks that they have a matching example in the examples folder actual test.
The thing is, that it's failing on the ElasticsearchHook which I deprecated (because I renamed it) in this PR. I don't want to add an example for a deprecated hook, because it kinda beets the purpose.. Do you have any tips?

Look at the tests - there some other deprecated classes there mentioned in the exclusion list.

alexkruc · 2022-07-08T09:14:16Z

The test TestElasticsearchProviderProjectStructure is failing. From what I understand, it looks for all the available hooks, and looks that they have a matching example in the examples folder actual test.
The thing is, that it's failing on the ElasticsearchHook which I deprecated (because I renamed it) in this PR. I don't want to add an example for a deprecated hook, because it kinda beets the purpose.. Do you have any tips?

Look at the tests - there some other deprecated classes there mentioned in the exclusion list.

Wow I completely missed the DEPRECATED_CLASSES somehow. Thanks for the tip! :) I'll try that out after the build (that will probably fail, but I want to see if the build docs part succeeds after the rebase) :)

…c/airflow into adding_elasticsearch_python_hook

alexkruc · 2022-07-08T12:59:19Z

@potiuk Sorry to trouble you again.. I followed your tips, and now the integration tests are passing! But the build docs are still failing on the same thing - /hooks/elasticsearch.rst.rst:37:non-whitespace stripped by dedent.
Do you have a tip regarding what's wrong there? Breeze static-checks and pre-commit are passing, and the dedent clause is the same as in other files (at least from what my IDE sees)..

Really appreciate your help 🙏

potiuk · 2022-07-08T14:17:25Z

Well. Not sure - seems like really "cryptic" error from Sphinx (which is known from rather cryptic messages). Look at the actual output of the sphix build (Above the summary) and you will find the warning there.

Also there is apparently a bug (@mik-laj?) that in this case our error parsing added second .rst to display the line of .rst file that was actually causing it.

However I made an intelligent guess and looked closely. From what I see, I think the lines below are all indented ONE SPACE MORE than all the others. so maybe fixing that will help.

    Connection types <connections/elasticsearch>
    Logging for Tasks <logging/index>
    Elasticsearch hook with native Python client <hooks/elasticsearch>

Also few other tips - and ways I deal with similar cases (been there, done that many times). You can easily iterate locally with it (and this is what I do when I have similar issues with docs):

breeze build-docs --package-filter apache-airlfow-provider-elasticsearch --docs-only

This will run much faster than CI circle and allows you to also take a look at sphinx-generated intermediate artifacts which usually helps with debugging it.

If all else fails. I usually fall-back to just looking at teh source code of Sphins and finding what the error message actually means (i.e. when it will be produced). In this case: WARNING: non-whitespace stripped by dedent - my intelligent guess is that Sphinx took the first indentation depth and stripped the same identation from the next block (which lead to stripping some non-white space character) - that's why the guess that one-space-more of indentation in the first block might be the reason. But I have not looked it up in the sources yet.

alexkruc · 2022-07-08T21:25:21Z

Finally, all the tests are passing! :D
To recap, the Sphinx error messages are ambiguous and don't point to the actual issues..
The issue with the dedent was real, but it referred to the indent of the example code. As I'm passing the Python block without any indents, I had to just remove the dedent clause from the rst file.

Later, I had issues with a "typo" - but Sphinx just set it as a generic typo, and did not always point to the correct file with the error..

In any case, at least now I know how to handle these issues - the more I know, the more I can help other people in the community :)

potiuk · 2022-07-08T21:51:05Z

To recap, the Sphinx error messages are ambiguous and don't point to the actual issues..

Yeah. quite often the message are rather unhelpful, I must agree.

Adding ElasticserachPythonHook - ES Hook With The Python Client

fa95ffb

alexkruc requested a review from mik-laj as a code owner July 7, 2022 08:53

boring-cyborg bot added area:providers area:system-tests kind:documentation labels Jul 7, 2022

eladkal self-requested a review July 7, 2022 08:56

eladkal reviewed Jul 7, 2022

View reviewed changes

Unifying the elasticsearch hooks into a single file

fa64f6b

eladkal reviewed Jul 7, 2022

View reviewed changes

alexkruc added 3 commits July 7, 2022 14:59

CR comments - merged examples, added documentations, cached connection

82f366d

static check fixes

fd55897

static check fixes - license and spacing

5100406

alexkruc added 5 commits July 8, 2022 11:00

Adding ElasticserachPythonHook - ES Hook With The Python Client

3f4270b

Unifying the elasticsearch hooks into a single file

3ae160f

CR comments - merged examples, added documentations, cached connection

e990eaf

static check fixes

98f6a8e

static check fixes - license and spacing

fa9d058

potiuk force-pushed the adding_elasticsearch_python_hook branch from 5100406 to fa9d058 Compare July 8, 2022 09:01

alexkruc and others added 3 commits July 8, 2022 12:50

Merge branch 'adding_elasticsearch_python_hook' of github.com:alexkru…

3eb060e

…c/airflow into adding_elasticsearch_python_hook

adding ElasticsearchHook to DEPRECATED_CLASSES

e0e7518

Merge branch 'main' into adding_elasticsearch_python_hook

b865178

alexkruc added 2 commits July 8, 2022 19:50

Changing docs structure

4508b61

Merge branch 'main' into adding_elasticsearch_python_hook

2b44522

alexkruc added 3 commits July 8, 2022 21:56

remove dedent from es hooks

07657c3

fixing found typo (?)

02ad7f4

fixing found typo again

0baff02

alexkruc requested a review from eladkal July 8, 2022 21:43

potiuk approved these changes Jul 8, 2022

View reviewed changes

potiuk merged commit 2ddc100 into apache:main Jul 8, 2022

potiuk mentioned this pull request Jul 13, 2022

Status of testing Providers that were prepared on July 13, 2022 #25037

Closed

95 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ElasticserachPythonHook - ES Hook With The Python Client #24895

Adding ElasticserachPythonHook - ES Hook With The Python Client #24895

alexkruc commented Jul 7, 2022 •

edited

Loading

eladkal Jul 7, 2022

alexkruc Jul 7, 2022

alexkruc Jul 7, 2022

eladkal Jul 7, 2022

alexkruc Jul 7, 2022

eladkal Jul 7, 2022

alexkruc Jul 7, 2022

eladkal Jul 7, 2022

alexkruc Jul 7, 2022

potiuk commented Jul 7, 2022

alexkruc commented Jul 8, 2022 •

edited

Loading

potiuk commented Jul 8, 2022

potiuk commented Jul 8, 2022

alexkruc commented Jul 8, 2022

alexkruc commented Jul 8, 2022

potiuk commented Jul 8, 2022

alexkruc commented Jul 8, 2022 •

edited

Loading

potiuk commented Jul 8, 2022

		@@ -0,0 +1,58 @@
		# Licensed to the Apache Software Foundation (ASF) under one

Adding ElasticserachPythonHook - ES Hook With The Python Client #24895

Adding ElasticserachPythonHook - ES Hook With The Python Client #24895

Conversation

alexkruc commented Jul 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Jul 7, 2022

alexkruc commented Jul 8, 2022 • edited Loading

potiuk commented Jul 8, 2022

potiuk commented Jul 8, 2022

alexkruc commented Jul 8, 2022

alexkruc commented Jul 8, 2022

potiuk commented Jul 8, 2022

alexkruc commented Jul 8, 2022 • edited Loading

potiuk commented Jul 8, 2022

alexkruc commented Jul 7, 2022 •

edited

Loading

alexkruc commented Jul 8, 2022 •

edited

Loading

alexkruc commented Jul 8, 2022 •

edited

Loading