Skip to content

Conversation

@trickysky
Copy link
Contributor

@trickysky trickysky commented Mar 17, 2023

BaseHook.get_connections is deprecated, only return one connection
snakebite HAClient need more than one connection

HAClient

@trickysky trickysky changed the title fix HDFSHook HAClient invalid Fix HDFSHook HAClient is invalid Mar 17, 2023
@trickysky
Copy link
Contributor Author

trickysky commented Mar 17, 2023

Another code implementation:
hdfs_conn_id is still only str type, two host join with comma, like "host1,host2"
if two port is difference, it's complex

@trickysky trickysky marked this pull request as draft March 17, 2023 09:37
@trickysky trickysky marked this pull request as ready for review March 17, 2023 09:38
@potiuk
Copy link
Member

potiuk commented Mar 21, 2023

I think we need a unit test covering it to prevent regressions. It's suspicious not see tests modified/added/fixed with such a change

@trickysky
Copy link
Contributor Author

I think we need a unit test covering it to prevent regressions. It's suspicious not see tests modified/added/fixed with such a change

I modify a bad unit test, @potiuk please take a look, thanks

def test_get_ha_client(self, mock_get_connections):
conn_1 = Connection(conn_id="hdfs_default", conn_type="hdfs", host="localhost", port=8020)
conn_2 = Connection(conn_id="hdfs_default", conn_type="hdfs", host="localhost2", port=8020)
mock_get_connections.return_value = [conn_1, conn_2]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Connection model conn_id field is unique

@potiuk
Copy link
Member

potiuk commented Mar 22, 2023

Needs static checks fixing though (you can install/run pre-commit to fix those).

@potiuk potiuk merged commit e141699 into apache:main Mar 27, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 27, 2023

Awesome work, congrats on your first merged pull request!

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.

2 participants