Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk merged PR #44718: URL: https://github.com/apache/airflow/pull/44718 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1883930019 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: > I liked the solution as to me, coming from Java, it seemed like a nice and clean solution. Yeah. It's nice, but it's more of a byproduct of what they have as an example usage. What happens if one day they release the package and the "synchronized" decorator will not be there or moved? They can do it even in patchlevel version. and claim "it's ok to move it - it's just an example" - we cannot rely on it being there. We could also write our own decorator (basically copy what they have done) and keep it in, but that seems an overkill if we can do the same with `self.lock =" and `with lock` two lines added. It's abit like infamous `leftpad` fiasco. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
dabla commented on PR #44718: URL: https://github.com/apache/airflow/pull/44718#issuecomment-2541445005 @potiuk modified the code as you asked I think this one is ok now -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
dabla commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881605066 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: > Hmm. I found it in https://wrapt.readthedocs.io/en/master/examples.html - but search did not found it. It's therefore an example in wrapt - which is also a bit strange to rely on ? I liked the solution as to me, coming from Java, it seemed like a nice a clean solution. You can use the decorator as an aspect which prevents other threads from entering until the current one has finished, no need to hard code this into the function as this is a crosscutting concern, imho. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
dabla commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881605066 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: > Hmm. I found it in https://wrapt.readthedocs.io/en/master/examples.html - but search did not found it. It's therefore an example in wrapt - which is also a bit strange to rely on ? I liked the solution as to me, coming from Java, it seemed like a nice and clean solution. You can use the decorator as an aspect which prevents other threads from entering until the current one has finished, no need to hard code this into the function as this is a crosscutting concern, imho. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
dabla commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881606525 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: > Possibly better to use with > > ``` > self.lock = threading.RLock() > > with self.lock: > jaydebeapi.connect( ... > ``` > > It's essentially the same but better expresses the fact that we want to synchronize jaydebeapi, not the `get_conn` method. I will adapt it like this then. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881040174 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: Possibly better to use with ``` self.lock = threading.RLock() with self.lock: jaydebeapi.connect( ... ``` It's essentially the same but better expresses the fact that we want to synchronize jaydebeapi, not the `get_conn` method. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881032784 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: Hmm. I found it in https://wrapt.readthedocs.io/en/master/examples.html - but search did not found it. It's therefore an example in wrapt - which is also a bit strange to rely on ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1881028446 ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: I have not found "synchronized" as something that wrapt would publish as an interface. It seems it is somewhat accidentally available in wrapt implementation and might change any time. I think we should find a more "official" tool to doing it. ## providers/src/airflow/providers/jdbc/hooks/jdbc.py: ## @@ -25,6 +25,7 @@ import jaydebeapi import jpype from sqlalchemy.engine import URL +from wrapt import synchronized Review Comment: I have not found "synchronized" as something that wrapt would publish as an interface. It seems it is somewhat accidentally available in wrapt implementation and might change any time. I think we should find a more "official" tool to do it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
dabla commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1874008395 ## providers/tests/jdbc/hooks/test_jdbc.py: ## @@ -54,10 +56,17 @@ def get_hook( **conn_params, } ) +jvm_started = False class MockedJdbcHook(JdbcHook): @classmethod def get_connection(cls, conn_id: str) -> Connection: +# nonlocal jvm_started Review Comment: Yeah forgot to remove it apologies -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]
potiuk commented on code in PR #44718: URL: https://github.com/apache/airflow/pull/44718#discussion_r1872869011 ## providers/tests/jdbc/hooks/test_jdbc.py: ## @@ -54,10 +56,17 @@ def get_hook( **conn_params, } ) +jvm_started = False class MockedJdbcHook(JdbcHook): @classmethod def get_connection(cls, conn_id: str) -> Connection: +# nonlocal jvm_started Review Comment: Is it commented intentionally ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org