Re: [PR] Made get_conn in JdbcHook threadsafe to avoid OSError: JVM is already started [airflow]

2024-12-13 Thread via GitHub


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]

2024-12-13 Thread via GitHub


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]

2024-12-13 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-12 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-11 Thread via GitHub


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]

2024-12-06 Thread via GitHub


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]

2024-12-06 Thread via GitHub


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