[AIRFLOW-3888] HA for metastore connection #4708

GuzikJakub · 2019-02-14T15:39:34Z

Jira

https://issues.apache.org/jira/browse/AIRFLOW-3888

Description

Creating a connection to a metastore with two hosts for high availability (eg connection 1, connection 2) is not possible because the entire value entered is taken. For our needs, it is necessary to go through subsequent hosts and connect to the first working.

This change allows you to check and then connect to a working metastor.

Tests

Tested on two metastores, of which one was always inactive

Example:
matastore1(offline); metastoer2(online).
In this case, we will connect to metastore2

Documentation

The connection is created in the webserver. We separate the metastore with a semicolon, eg: metastror1; metastore2; metastore3

Creating a connection to a metasotor with two hosts for high avitablity (eg connection 1, connection 2) is not possible because the entire value entered is taken. For our needs, it is necessary to go through subsequent hosts and connect to the first working. This change allows you to check and then connect to a working metastor.

mik-laj · 2019-02-14T17:17:19Z

Many connections with the same conn_id can be defined and when that is the case, and when the hooks uses the get_connection method from BaseHook, Airflow will choose one connection randomly, allowing for some basic load balancing and fault tolerance when used in conjunction with retries.

https://airflow.readthedocs.io/en/latest/concepts.html

Why did you create a new way to define multiple connections instead of expanding the old one? Why is your solution better? Could you complete the documentation?

GuzikJakub · 2019-02-15T10:45:25Z

Why did you create a new way to define multiple connections instead of expanding the old one? Why is your solution better? Could you complete the documentation?

The solution prepared by me is a little better, because I am based on checking the correctness of the connection to the metastore during the task and it is not necessary to repeat it in case of hitting the offline connection. Secondly, I do not choose connections randomly, but only check them in succession, as they were entered, thanks to which I have 100% certainty of the task in case of at least one working connection.
I have carried out the following test: I set the possibility of one additional attempt to perform a task that checks the existence of a partition in the hive. Next, I set 2 connections to the metastore (one online, the other offline). For 15 trials, only 75% of the tasks were passed correctly. On the other hand, more than 50% performed only during the second attempt. In my solution, each time it will be done during the first attempt (unless all connections are offline) - which may be a plus when calling such a connection in the middle of calculations, for example in PythonOperator (does not re-execute previous queries).

mik-laj · 2019-02-16T03:43:23Z

I prepared a PR with my other idea: https://github.com/GuzikJakub/airflow/pull/1

Uses AA mechanisms to store multiple connections rather than specific to this particular hook. In addition, I changed the way of logging in to the correct one (LoggingMixin instead logging). And also created a method to increase the readability of the code.

airflow/hooks/hive_hooks.py

Thank you. I made a few changes because during the tests I detected several errors. I have a question, when I do marge to my pull it will be still possible to land it in the airflow main branch?

HA for Metastore

flake8 code repair

Flake8 repair

Fokko · 2019-02-27T15:21:40Z

@gglanzani PTAL

gglanzani · 2019-02-28T10:51:52Z

airflow/hooks/hive_hooks.py

+            self.log.info("Trying to connect to %s:%s", conn.host, conn.port)
+            try:
+                result = host_socket.connect_ex((conn.host, conn.port))
+                host_socket.close()


We can rewrite the try like this

if host_socket.connect_ex((conn.host, conn.port)) == 0: self.log.info("Connected to %s:%s", conn.host, conn.port) host_socket.close() return conn

and avoid the break, the results and valid_conn variable, etc.

Code corrected and thrown. The wider answer will be prepared below.

I also removed variables such as break, results and valid_conn

gglanzani · 2019-02-28T10:52:13Z

airflow/hooks/hive_hooks.py

+                result = host_socket.connect_ex((conn.host, conn.port))
+                host_socket.close()
+            except Exception:
+                pass


Why are we swallowing this one without at least logging?

I understand that one of the two hosts might fail, but if both fail I won't see anything in the logs.

Test (but I made some corrections to the code):
[2019-03-04 14:08:51,954] {hive_hooks.py:528} INFO - Trying to connect to Metastore1
[2019-03-04 14:08:54,960] {hive_hooks.py:534} INFO - Could not connect to Metastore1
[2019-03-04 14:08:54,960] {hive_hooks.py:528} INFO - Trying to connect to Metastore2
[2019-03-04 14:08:54,962] {hive_hooks.py:534} INFO - Could not connect to Metastore2
[2019-03-04 14:08:54,970] {models.py:1760} ERROR - Failed to locate the valid server.
Traceback (most recent call last):
File "/airflow/models.py", line 1659, in _run_raw_task
result = task_copy.execute(context=context)
File "/airflow/sensors/base_sensor_operator.py", line 68, in execute
while not self.poke(context):
File "/airflow/sensors/hive_partition_sensor.py", line 73, in poke
metastore_conn_id=self.metastore_conn_id)
File "/airflow/hooks/hive_hooks.py", line 468, in init
self.metastore = self.get_metastore_client()
File "/airflow/hooks/hive_hooks.py", line 491, in get_metastore_client
raise AirflowException("Failed to locate the valid server.")
airflow.exceptions.AirflowException: Failed to locate the valid server.
[2019-03-04 14:08:54,971] {models.py:1789} INFO - All retries failed; marking task as FAILED

gglanzani · 2019-02-28T12:10:46Z

@GuzikJakub In principle LGTM, I've requested some changes to make the code easier (to understand) and for logging purposes.

It would be nice to add a test (possibly connecting to a port where nothing is listening?).

@Fokko what's your opinion on the test?

Code behavior improvements

GuzikJakub · 2019-03-04T13:21:44Z

@gglanzani I introduced the changes mentioned above. I have also added a test response that sends an error if there is no valid connection.

Fokko · 2019-03-04T13:24:22Z

@GuzikJakub I think you still need to git add the test :)

GuzikJakub · 2019-03-05T13:29:25Z

I have one small question. While I wrote unittest for the wrong port, how can I add a new basic connection in the test base? I need it because I can not pass a variable there myself - they are passed straight from the base.
In addition, I did not see any example of unittest, which checks the wrong connection (or I just did not find)

mik-laj · 2019-03-05T16:25:01Z

@GuzikJakub You must mock the method to return the expected value.
Look at: https://github.com/apache/airflow/blob/master/tests/contrib/hooks/test_gcp_vision_hook.py

Add test

test improvement

GuzikJakub · 2019-03-27T07:57:13Z

@gglanzani @Fokko Is it correct now?

gglanzani · 2019-03-27T21:13:03Z

@GuzikJakub The tests are still failing 🤔

Add test [AIRFLOW-3888] HA for metastore connection test improvement

Add test [AIRFLOW-3888] HA for metastore connection test improvement [AIRFLOW-3888] HA for metastore connection test improvement

pull rebase

Improving the typo in the variable name

gglanzani · 2019-03-29T19:35:44Z

tests/hooks/test_hive_hook.py

@@ -282,6 +283,13 @@ def test_get_max_partition_from_valid_part_specs(self):
    def test_get_metastore_client(self):
        self.assertIsInstance(self.hook.get_metastore_client(), HMSClient)

+    @mock.patch("airflow.hooks.hive_hooks.HiveMetastoreHook.get_connection",
+                **{'return_value': [Connection(host="localhost", port=9802)]})


Why not return_value=[Connection(…)] instead of using the ** syntax?

Sugested syntax does not work properly in all cases supported by mock. I often use only one syntax, so that I do not have to think which is right at the moment.
Reference:
https://github.com/apache/airflow/blob/49b98ab2b5dc37c69598d485d62dd39372ab6aa9/tests/contrib/hooks/test_gcp_natural_language_hook.py#L57-L68

Your scientists were so preoccupied with whether or not they could, they didn’t stop to think if they should.

The suggested syntax does not work, indeed, when you have invalid identifiers as keyword arguments. return_value is not one of them

gglanzani · 2019-03-29T19:36:21Z

Hi @GuzikJakub, beside a very minor comment, LGTM.

Mock return_value edit

Flake8 repair

Test repair

gglanzani · 2019-04-04T14:12:43Z

Tests failure seems unrelated, can you take a look @Fokko?

mik-laj · 2019-04-04T14:24:02Z

The added tests have a small coverage. Is it possible to add more tests?

GuzikJakub · 2019-04-05T09:08:43Z

@mik-laj In my opinion, the test I added includes everything that should be in this change. I check if the connection is correct (it was ready) and if it is not correct (added by me).

Fokko · 2019-04-05T12:39:02Z

@GuzikJakub Please rebase on master

Flake8 repair [AIRFLOW-3888] HA for metastore connection Test repair

GuzikJakub · 2019-04-05T14:50:53Z

@Fokko I did it

gglanzani · 2019-04-08T08:47:58Z

LGTM now

GuzikJakub · 2019-04-30T09:02:07Z

I am reminded about PR :)

Fokko · 2019-04-30T09:38:26Z

@XD-DENG any further questions?

XD-DENG · 2019-04-30T09:40:15Z

Hi @Fokko , no other comment from my side. Thanks!

* HA for Metastore * [AIRFLOW-3888] HA for metastore connection Creating a connection to a metasotor with two hosts for high avitablity (eg connection 1, connection 2) is not possible because the entire value entered is taken. For our needs, it is necessary to go through subsequent hosts and connect to the first working. This change allows you to check and then connect to a working metastor. * add function to base_hook * update webhdfs_hook * back to original version * back to original version * Update hive_hooks.py Thank you. I made a few changes because during the tests I detected several errors. I have a question, when I do marge to my pull it will be still possible to land it in the airflow main branch? * [AIRFLOW-3888] HA for metastore connection flake8 code repair * [AIRFLOW-3888] HA for metastore connection Flake8 repair * [AIRFLOW-3888] HA for metastore connection Code behavior improvements * [AIRFLOW-3888] HA for metastore connection Add test * [AIRFLOW-3888] HA for metastore connection test improvement * [AIRFLOW-3888] HA for metastore connection Add test [AIRFLOW-3888] HA for metastore connection test improvement * [AIRFLOW-3888] HA for metastore connection Add test [AIRFLOW-3888] HA for metastore connection test improvement [AIRFLOW-3888] HA for metastore connection test improvement * [AIRFLOW-3888] HA for metastore connection Improving the typo in the variable name * [AIRFLOW-3888] HA for metastore connection Mock return_value edit * [AIRFLOW-3888] HA for metastore connection Flake8 repair * [AIRFLOW-3888] HA for metastore connection Test repair * [AIRFLOW-3888] HA for metastore connection Flake8 repair [AIRFLOW-3888] HA for metastore connection Test repair

Creating a connection to a metastore with two hosts for high availability (eg connection 1, connection 2) is not possible because the entire value entered is taken. For our needs, it is necessary to go through subsequent hosts and connect to the first working. This change allows you to check and then connect to a working metastore. (cherry picked from commit 355bd56)

Kamil Breguła and others added 2 commits January 1, 2000 00:00

HA for Metastore

5d125ba

GuzikJakub added 4 commits February 15, 2019 12:58

add function to base_hook

949dcca

update webhdfs_hook

093d27f

back to original version

575be7c

back to original version

22fbd94

XD-DENG requested changes Feb 16, 2019

View reviewed changes

airflow/hooks/hive_hooks.py Outdated Show resolved Hide resolved

GuzikJakub added 4 commits February 19, 2019 12:21

Update hive_hooks.py

6cfbfde

Thank you. I made a few changes because during the tests I detected several errors. I have a question, when I do marge to my pull it will be still possible to land it in the airflow main branch?

Merge pull request #1 from mik-laj/ha-metastore

92b1967

HA for Metastore

[AIRFLOW-3888] HA for metastore connection

dbd8568

flake8 code repair

[AIRFLOW-3888] HA for metastore connection

5baff3d

Flake8 repair

gglanzani suggested changes Feb 28, 2019

View reviewed changes

[AIRFLOW-3888] HA for metastore connection

200829f

Code behavior improvements

GuzikJakub added 2 commits March 26, 2019 14:58

[AIRFLOW-3888] HA for metastore connection

adbe672

Add test

[AIRFLOW-3888] HA for metastore connection

9426148

test improvement

GuzikJakub and others added 4 commits March 28, 2019 12:34

[AIRFLOW-3888] HA for metastore connection

afd0f0c

Add test [AIRFLOW-3888] HA for metastore connection test improvement

Merge branch 'master' of https://github.com/GuzikJakub/airflow

77a2292

[AIRFLOW-3888] HA for metastore connection

6dd78b1

Add test [AIRFLOW-3888] HA for metastore connection test improvement [AIRFLOW-3888] HA for metastore connection test improvement

Merge branch 'master' of https://github.com/GuzikJakub/airflow

237d209

pull rebase

[AIRFLOW-3888] HA for metastore connection

52bb24e

Improving the typo in the variable name

gglanzani reviewed Mar 29, 2019

View reviewed changes

GuzikJakub added 3 commits April 3, 2019 11:31

[AIRFLOW-3888] HA for metastore connection

e22fea6

Mock return_value edit

[AIRFLOW-3888] HA for metastore connection

ccba96b

Flake8 repair

[AIRFLOW-3888] HA for metastore connection

4168103

Test repair

GuzikJakub and others added 2 commits April 5, 2019 14:46

[AIRFLOW-3888] HA for metastore connection

f57e006

Flake8 repair [AIRFLOW-3888] HA for metastore connection Test repair

rebase master

d93c5a6

Fokko requested a review from XD-DENG April 12, 2019 07:59

XD-DENG approved these changes Apr 30, 2019

View reviewed changes

Fokko merged commit 355bd56 into apache:master May 10, 2019

ashb mentioned this pull request Apr 28, 2020

Make conn_id unique in Connections table #8608

Closed

Aakcht mentioned this pull request Nov 23, 2021

hive provider: restore HA support for metastore #19777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3888] HA for metastore connection #4708

[AIRFLOW-3888] HA for metastore connection #4708

GuzikJakub commented Feb 14, 2019 •

edited

Loading

mik-laj commented Feb 14, 2019

GuzikJakub commented Feb 15, 2019

mik-laj commented Feb 16, 2019

Fokko commented Feb 27, 2019

gglanzani Feb 28, 2019

GuzikJakub Mar 4, 2019

gglanzani Feb 28, 2019

GuzikJakub Mar 4, 2019

gglanzani commented Feb 28, 2019

GuzikJakub commented Mar 4, 2019

Fokko commented Mar 4, 2019

GuzikJakub commented Mar 5, 2019

mik-laj commented Mar 5, 2019

GuzikJakub commented Mar 27, 2019

gglanzani commented Mar 27, 2019

gglanzani Mar 29, 2019

mik-laj Mar 29, 2019 •

edited

Loading

gglanzani Mar 29, 2019

gglanzani commented Mar 29, 2019

gglanzani commented Apr 4, 2019

mik-laj commented Apr 4, 2019

GuzikJakub commented Apr 5, 2019

Fokko commented Apr 5, 2019

GuzikJakub commented Apr 5, 2019

gglanzani commented Apr 8, 2019

GuzikJakub commented Apr 30, 2019

Fokko commented Apr 30, 2019

XD-DENG commented Apr 30, 2019

[AIRFLOW-3888] HA for metastore connection #4708

[AIRFLOW-3888] HA for metastore connection #4708

Conversation

GuzikJakub commented Feb 14, 2019 • edited Loading

Jira

Description

Tests

Documentation

mik-laj commented Feb 14, 2019

GuzikJakub commented Feb 15, 2019

mik-laj commented Feb 16, 2019

Fokko commented Feb 27, 2019

gglanzani Feb 28, 2019

Choose a reason for hiding this comment

GuzikJakub Mar 4, 2019

Choose a reason for hiding this comment

gglanzani Feb 28, 2019

Choose a reason for hiding this comment

GuzikJakub Mar 4, 2019

Choose a reason for hiding this comment

gglanzani commented Feb 28, 2019

GuzikJakub commented Mar 4, 2019

Fokko commented Mar 4, 2019

GuzikJakub commented Mar 5, 2019

mik-laj commented Mar 5, 2019

GuzikJakub commented Mar 27, 2019

gglanzani commented Mar 27, 2019

gglanzani Mar 29, 2019

Choose a reason for hiding this comment

mik-laj Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

gglanzani Mar 29, 2019

Choose a reason for hiding this comment

gglanzani commented Mar 29, 2019

gglanzani commented Apr 4, 2019

mik-laj commented Apr 4, 2019

GuzikJakub commented Apr 5, 2019

Fokko commented Apr 5, 2019

GuzikJakub commented Apr 5, 2019

gglanzani commented Apr 8, 2019

GuzikJakub commented Apr 30, 2019

Fokko commented Apr 30, 2019

XD-DENG commented Apr 30, 2019

GuzikJakub commented Feb 14, 2019 •

edited

Loading

mik-laj Mar 29, 2019 •

edited

Loading