dpgaspar commented on code in PR #35081:
URL: https://github.com/apache/superset/pull/35081#discussion_r2337274909
##########
superset/annotation_layers/annotations/api.py:
##########
@@ -300,11 +300,10 @@ def post(self, pk: int) -> Response: # pylint:
disable=arguments-differ
except AnnotationInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except AnnotationCreateFailedError as ex:
- logger.error(
+ logger.exception(
"Error creating annotation %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
using `logger.exception` I think we can remove str(ex) it's actually not a
good practice
##########
superset/annotation_layers/annotations/api.py:
##########
@@ -374,11 +373,10 @@ def put( # pylint: disable=arguments-differ
except AnnotationInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except AnnotationUpdateFailedError as ex:
- logger.error(
+ logger.exception(
"Error updating annotation %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
using `logger.exception` I think we can remove `str(ex)`
##########
superset/annotation_layers/api.py:
##########
@@ -152,11 +152,10 @@ def delete(self, pk: int) -> Response:
except AnnotationLayerDeleteIntegrityError as ex:
return self.response_422(message=str(ex))
except AnnotationLayerDeleteFailedError as ex:
- logger.error(
+ logger.exception(
"Error deleting annotation layer %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
same as above
##########
superset/common/utils/query_cache_manager.py:
##########
@@ -183,10 +183,9 @@ def get(
current_app.config["STATS_LOGGER"].incr("loaded_from_cache")
except KeyError as ex:
logger.exception(ex)
- logger.error(
+ logger.exception(
"Error reading cache: %s",
error_msg_from_exception(ex),
Review Comment:
we already logged an exception, so or we remove the logger error here, or
keep it on this case
##########
superset/connectors/sqla/utils.py:
##########
@@ -197,7 +197,7 @@ def find_cached_objects_in_session(
try:
items = list(db.session)
except ObjectDeletedError:
- logger.warning("ObjectDeletedError", exc_info=True)
+ logger.exception("ObjectDeletedError")
Review Comment:
keep the warning level?
##########
superset/daos/dataset.py:
##########
@@ -44,7 +44,7 @@ def get_database_by_id(database_id: int) -> Database | None:
try:
return
db.session.query(Database).filter_by(id=database_id).one_or_none()
except SQLAlchemyError as ex: # pragma: no cover
- logger.error("Could not get database by id: %s", str(ex),
exc_info=True)
+ logger.exception("Could not get database by id: %s", str(ex))
Review Comment:
remove str(ex)?
##########
superset/connectors/sqla/models.py:
##########
@@ -1637,9 +1637,7 @@ def assign_column_label(df: pd.DataFrame) -> pd.DataFrame
| None:
# everything else / the default should be to let things bubble up.
df = pd.DataFrame()
status = QueryStatus.FAILED
- logger.warning(
- "Query %s on schema %s failed", sql, self.schema, exc_info=True
- )
+ logger.exception("Query %s on schema %s failed", sql, self.schema)
Review Comment:
maybe we should keep warnings as warnings, may show too many errors on log
platforms and trigger unnecessary alarms
##########
superset/annotation_layers/api.py:
##########
@@ -216,11 +215,10 @@ def post(self) -> Response:
except AnnotationLayerInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except AnnotationLayerCreateFailedError as ex:
- logger.error(
+ logger.exception(
"Error creating annotation %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
same as above
##########
superset/annotation_layers/api.py:
##########
@@ -287,11 +285,10 @@ def put(self, pk: int) -> Response:
except AnnotationLayerInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except AnnotationLayerUpdateFailedError as ex:
- logger.error(
+ logger.exception(
"Error updating annotation %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
same as above
##########
superset/annotation_layers/annotations/api.py:
##########
@@ -428,11 +426,10 @@ def delete( # pylint: disable=arguments-differ
except AnnotationNotFoundError:
return self.response_404()
except AnnotationDeleteFailedError as ex:
- logger.error(
+ logger.exception(
"Error deleting annotation %s: %s",
self.__class__.__name__,
str(ex),
Review Comment:
same as above
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
