Re: [PR] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk merged PR #38707: URL: https://github.com/apache/airflow/pull/38707 -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1562316577
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
Yep. One day.
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1561754471
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
The more I discover the code the more I think: "hey I saw this before, it
would be nice to have that as a common util code". Ofc there is the dependency
issue, it's always a trade-off at some point, unless we could like include code
dynamically like @potiuk said, but dunno if that's possible in Python. In Java
you could achieve a bit the same with an aspectj compiler at build phase. Just
wondering, if that new common util package would have no airflow dependencies,
would it then still be an issue of dependency hell, as the origin of the
dependency issues are due to the different airflow versions no? But if the new
package doens't depend on airflow (dunno if possible would have to try it),
then new providers would only have an extra dependency on that package. Or wy
try a PR as draft and see what comes out of it dunno what you guys think, at
some moment we'll have to think for a descent solution anyway.
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
Taragolis commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1561583638
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
There is two opinions fights to each other inside me:
First one told me: "hey! It would be awesome! It'll reduce codebase and
might solve common mistake"
The second one whisper: "look at common sql, one simple change in common
utility and it introduce bug in 50+ providers"
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1561336492
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
We can definitely start with copying and the "common.utils" might come later
- I am fine wit poison 1) and copying stuff. Definitely it's not worth
introducing the common.util kind of approach for that single method, buut we
can later decide on common.utils and maybe a lot more common things. Or maybe
we decide not to use it at all (because yes - it introduces quite some hassle)
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
Taragolis commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1561222877
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
>Ok. then what we should do if it is used in several providers - since we
have no common.util - do you propose to copy it simply ? Less DRY, less
coupling, more copy&paste. Or any other idea?
My proposal do not mix-in new feature changes in two different provider in
the same time.
- We do not know is it become useful into the other provider and would be
- We do not know side effect (in general, not in this case)
So if the purpose it to do into the single one/two provider, keep it in it
do not need to put it into the `common.sql` it is not **common** and it is not
**sql**
No required to replace one potential `common.utils` package by the other
`common.sql`, no additional headache to release manager, because it required to
bump version of dependency, and until last time it might be unknow which next
version would be patch, minor and major
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560920195
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
I'm willing to start a new PR regarding option 3, as I have a feeling you
guys prefer that approach, but I don't know if I will be able to lead it but
can try. I would have done it as option 2 but 3 is also fine for me, as long
as at the end we come to a cleaner and DRY solution.
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560917676
##
tests/providers/common/sql/hooks/test_sql.py:
##
@@ -251,3 +252,12 @@ def
test_make_common_data_structure_no_deprecated_method(self):
def test_placeholder_config_from_extra(self):
dbapi_hook = mock_hook(DbApiHook, conn_params={"extra":
{"placeholder": "?"}})
assert dbapi_hook.placeholder == "?"
+
+def test_suppress_and_warn_when_raised_exception_is_suppressed(self):
Review Comment:
done
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560914208
##
tests/providers/common/sql/hooks/test_sql.py:
##
@@ -251,3 +252,12 @@ def
test_make_common_data_structure_no_deprecated_method(self):
def test_placeholder_config_from_extra(self):
dbapi_hook = mock_hook(DbApiHook, conn_params={"extra":
{"placeholder": "?"}})
assert dbapi_hook.placeholder == "?"
+
+def test_suppress_and_warn_when_raised_exception_is_suppressed(self):
Review Comment:
Yes I thought I reverted that one as I've adapted it in the wrong branch,
it's reverted 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049539299 > Also `pre-commit` needs to be run @dabla, see the error here: https://github.com/apache/airflow/actions/runs/8645779337/job/23703693429?pr=38707 Yes I thought I reverted that one as I've adapted it in the wrong branch, it's reverted 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560914208
##
tests/providers/common/sql/hooks/test_sql.py:
##
@@ -251,3 +252,12 @@ def
test_make_common_data_structure_no_deprecated_method(self):
def test_placeholder_config_from_extra(self):
dbapi_hook = mock_hook(DbApiHook, conn_params={"extra":
{"placeholder": "?"}})
assert dbapi_hook.placeholder == "?"
+
+def test_suppress_and_warn_when_raised_exception_is_suppressed(self):
Review Comment:
Yes I thought I reverted that one as I've adapted it in the wrong branch,
it's reverted 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049478718 Also `pre-commit` needs to be run @dabla, see the error here: https://github.com/apache/airflow/actions/runs/8645779337/job/23703693429?pr=38707 -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560846808
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
(BTW. I am also super happy to help in implementing/reviewing 3) as long as
somoene takes a lead there)
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560844228
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
Yes, that was the idea of my intial proposal - to move it there as it would
be immediately useful for all those 24 providers.
And yes - I agree with @Taragolis that this is more suitable for
`common.utils` or whatever we might come up with - but well, we do not have
`common.utils` yet nor even the consensus of whether we need it and how to
manage it. There was some vague idea about polyfills in the devlist discuussion
(but I have no idea how to do it in the Python world and what it means for our
build and release process).
So we have no "perfect" solution - we have only "so-so" ones.
1) make it non-DRY and copy
2) reuse it via common.sql
3) (most complex) agree to, create and use `common.utils`
I am good with all three approaches. As long as someone will take on the 3)
and prepares and takes lead of common.utils - I think it's the right time I
stop leading and implementing this all myself.
I guess @Taragolis - pick your poison.
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560828589
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
@jarek @Taragolis It could be nice thought to have the suppress_and_warn
method in the common sql provider, as we could then also reuse it (after some
refactoring allowing a custom warning message to be passed) in the following
method and then we would be DRY (e.g. not repeating ourselves):
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with contextlib.suppress(AttributeError):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
warnings.warn(
"The `_make_serializable` method is deprecated and support
will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
AirflowProviderDeprecationWarning,
stacklevel=2,
)
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
It could become this then:
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with suppress_and_warn(
AttributeError,
message="The `_make_serializable` method is deprecated and
support will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
stacklevel=2,
):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
What do you think? Ofc drawback is the dependency, but all db related hooks
are already dependent on the common sql anyway no?
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560828589
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
@jarek @Taragolis It could be nice thought to have the suppress_and_warn
method in the common sql provider, as we could then also reuse it (after some
refactoring allowing a custom warning message to be passed) in the following
method and then we would be DRY (e.g. not repeating ourselves):
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with contextlib.suppress(AttributeError):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
warnings.warn(
"The `_make_serializable` method is deprecated and support
will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
AirflowProviderDeprecationWarning,
stacklevel=2,
)
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
It could become this then:
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with suppress_and_warn(
AttributeError,
message="The `_make_serializable` method is deprecated and
support will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
stacklevel=2,
):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
What do you think? Ofc drawback is the dependency, but all db related hooks
are already dependent on the common sql anyway no?
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560828589
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
It could be nice thought to have the suppress_and_warn method in the common
sql provider, as we could then also reuse it (after some refactoring allowing a
custom warning message to be passed) in the following method and then we would
be DRY (e.g. not repeating ourselves):
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with contextlib.suppress(AttributeError):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
warnings.warn(
"The `_make_serializable` method is deprecated and support
will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
AirflowProviderDeprecationWarning,
stacklevel=2,
)
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
It could become this then:
```
def _make_common_data_structure(self, result: T | Sequence[T]) -> tuple
| list[tuple]:
"""Ensure the data returned from an SQL command is a standard tuple
or list[tuple].
This method is intended to be overridden by subclasses of the
`DbApiHook`. Its purpose is to
transform the result of an SQL command (typically returned by cursor
methods) into a common
data structure (a tuple or list[tuple]) across all DBApiHook derived
Hooks, as defined in the
ADR-0002 of the sql provider.
If this method is not overridden, the result data is returned as-is.
If the output of the cursor
is already a common data structure, this method should be ignored.
"""
# Back-compatibility call for providers implementing old
´_make_serializable' method.
with suppress_and_warn(
AttributeError,
message="The `_make_serializable` method is deprecated and
support will be removed in a future "
f"version of the common.sql provider. Please update the
{self.__class__.__name__}'s provider "
"to a version based on common.sql >= 1.9.1.",
stacklevel=2,
):
result = self._make_serializable(result=result) # type:
ignore[attr-defined]
if isinstance(result, Sequence):
return cast(List[tuple], result)
return cast(tuple, result)
```
What do you think?
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560823864
##
tests/providers/common/sql/hooks/test_sql.py:
##
@@ -251,3 +252,12 @@ def
test_make_common_data_structure_no_deprecated_method(self):
def test_placeholder_config_from_extra(self):
dbapi_hook = mock_hook(DbApiHook, conn_params={"extra":
{"placeholder": "?"}})
assert dbapi_hook.placeholder == "?"
+
+def test_suppress_and_warn_when_raised_exception_is_suppressed(self):
Review Comment:
If we limit it to **only** jdbc - that one needs to be moved too :D
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560768880
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
I moved it to jdbc hook
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560753876
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
Ok. then what we should do if it is used in several providers - since we
have no `common.util` - do you propose to copy it simply ? Less DRY, less
coupling, more copy&paste. Or any other idea?
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
Taragolis commented on code in PR #38707:
URL: https://github.com/apache/airflow/pull/38707#discussion_r1560717254
##
airflow/providers/common/sql/hooks/sql.py:
##
@@ -111,6 +112,15 @@ def fetch_one_handler(cursor) -> list[tuple] | None:
return None
+@contextmanager
+def suppress_and_warn(*exceptions: type[BaseException]):
+"""Context manager that suppresses the given exceptions and logs a warning
message."""
+try:
+yield
+except exceptions as e:
+warnings.warn(f"Exception suppressed: {e}\n{traceback.format_exc()}",
category=UserWarning)
+
+
Review Comment:
This one doesn't use in `common.sql`, so it should be moved into the
specific provider, rather than keep it in `common.sql`
--
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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049160147 Yes. But don't do it yet indeed. We have no policy for that and this is something I just realised we will need to fix - becuase some of the providers **might** depend on features that were released since their min version. In most installation they already have latest common.sql because it is set like that in constraints in the new -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049133705 @potiuk You're sure I need to update the dependency in all providers? Because I checked and I see not all providers depend on the same version. I just ask to be sure so I don't break any provider.  -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049125409 > 2\. common-sql-provider dependencies Ok thanks @potiuk will do that 👍 Conflict has already been resolved -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2049088351 You will also need to rebase and solve sql.pyi conflict (after stub generation has been fixed in https://github.com/apache/airflow/pull/38915. Also. I think it's best (and I think we are going to propose and automated a policy about it) you will need to: 1) update common-sql provider version to 1.12 (otherwise you will get a dependency error in the CI) - this is done by adding 1.12.0 to provider.yaml 2) update common-sql-provider dependencies everywhere else to >= 1.12 (in all other provider.yaml) - this will avoid accidental using of new feature when older common.sql provider is installed. -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2048986873 Ok I moved the method to the common sql provider and adapted the dependency version of apache-airflow-providers-common-sql in jdbc/provider.yaml but I just don't find where I can add that in the documentation when this new method was added. -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2048968598 > > Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :( > > Simply - You can't use code from `airfliow` in providers until the provider has `apache-airflow>=NEXT_MINOR` - until then the provider must have a "polyfill" - i.e. catch import error and have the same code that is in the provider that providers same functionality when provider is installed on airflow < NEXT_MINOR. With a note to remove it when min-airflow version is >= NEXT_MINOR (we have a policy for bumping - in two weeks we increase min-airflow-version to 2.7.0, so after that any code that was there fore < 2.7 can be removed from providers. > > For SQL providers, a simpler way around it is to add a code to common.sql as a new feature and use `apache-airfow-provider-common-sql >= in `provider.yaml` - but then it should be visibly marked as added in common.sql x.y.z (via since flag in the docuemntation). > > There was a discussion to have `common.util` provider at some point of time but it stalled a bit. Maybe we should come back to it. Ok got it, thank you for the explantion, indeed I saw that discussion of the common.util. -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2048066159 > Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :( Simply - You can't use code from `airfliow` in providers until the provider has `apache-airflow>=NEXT_MINOR` - until then the provider must have a "polyfill" - i.e. catch import error and have the same code that is in the provider that providers same functionality when provider is installed on airflow < NEXT_MINOR. With a note to remove it when min-airflow version is >= NEXT_MINOR (we have a policy for bumping - in two weeks we increase min-airflow-version to 2.7.0, so after that any code that was there fore < 2.7 can be removed from providers. For SQL providers, a simpler way around it is to add a code to common.sql as a new feature and use `apache-airfow-provider-common-sql >= in `provider.yaml` - but then it should be visibly marked as added in common.sql x.y.z (via since flag in the docuemntation). There was a discussion to have `common.util` provider at some point of time but it stalled a bit. Maybe we should come back to 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on PR #38707: URL: https://github.com/apache/airflow/pull/38707#issuecomment-2048041234 > There is a problem with back-compatibility. Our providers work for `Airflow >= 2.6.0` - see the errrors raised in tests - it cannot be imported from Airlfow until we keep 2.10.0 support - you will need to duplicate it in the provider and fall-back to it and mark it as "remove after min-airflow version is set to 2.10. > > Another option (probably better) is to add it to common.sql and use from there (and add common.sql >= NEXT MINOR VERSION in provider.yaml. Yes I saw that and I have same issue I think with MSGraphOperator but I don't fully understand how I can fix this :( -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla commented on code in PR #38707: URL: https://github.com/apache/airflow/pull/38707#discussion_r1557020070 ## airflow/providers/jdbc/hooks/jdbc.py: ## @@ -152,7 +153,8 @@ def set_autocommit(self, conn: jaydebeapi.Connection, autocommit: bool) -> None: :param conn: The connection. :param autocommit: The connection's autocommit setting. """ -conn.jconn.setAutoCommit(autocommit) +with suppress(jaydebeapi.Error): +conn.jconn.setAutoCommit(autocommit) Review Comment: Yes indeed that would be better will do that, I assumed the suppress would already do that but that's not the case indeed. -- 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] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
potiuk commented on code in PR #38707: URL: https://github.com/apache/airflow/pull/38707#discussion_r1555010713 ## airflow/providers/jdbc/hooks/jdbc.py: ## @@ -152,7 +153,8 @@ def set_autocommit(self, conn: jaydebeapi.Connection, autocommit: bool) -> None: :param conn: The connection. :param autocommit: The connection's autocommit setting. """ -conn.jconn.setAutoCommit(autocommit) +with suppress(jaydebeapi.Error): +conn.jconn.setAutoCommit(autocommit) Review Comment: Should we print a warning instead? That seems like pretty unexpected state and might be difficult to debug problems when someone expects autocommit to be effective. -- 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
[PR] Suppress jaydebeapi.Error when setAutoCommit or getAutoCommit is unsupported by JDBC driver [airflow]
dabla opened a new pull request, #38707:
URL: https://github.com/apache/airflow/pull/38707
We experienced unexpected exceptions being raised by the JdbcHook as it is
invoking the setAutoCommit and getAutoCommit methods on the JDBC driver, but in
our case as we are using the SAS JDBC driver, those methods throw an
UnsupportedOperationException in Java. Even though those methods make part of
the JDBC spec, it isn't always implemented by the driver hence why the
exception. So to avoid those exceptions to be propagated, it's better to
suppress them.
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
