You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/16 17:53:32 UTC
[GitHub] [incubator-superset] john-bodley opened a new pull request #9560:
[fix] warm up cache error handling
john-bodley opened a new pull request #9560: [fix] warm up cache error handling
URL: https://github.com/apache/incubator-superset/pull/9560
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
This PR checks ensures that a slice which fails to be successfully warmed is reported. Note the existing error handling for the API endpoint is somewhat inconsistent, i.e,. it short circuits and returns a 500 if a slice fail to warm. Long term I'm not certain if the response should be keyed by slice ID with the status.
### TEST PLAN
Tested locally and error errors were being propagated for failed queries.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
to: @etr2460 @graceguo-supercat
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9560: [fix] warm up cache error handling
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9560: [fix] warm up cache error handling
URL: https://github.com/apache/incubator-superset/pull/9560#discussion_r409794328
##########
File path: superset/views/core.py
##########
@@ -1707,15 +1709,16 @@ def warm_up_cache(self):
form_data=form_data,
force=True,
)
- obj.get_json()
+ payload = obj.get_payload()
+ error = payload["error"]
+ status = payload["status"]
except Exception as ex:
- logger.exception("Failed to warm up cache")
- return json_error_response(utils.error_msg_from_exception(ex))
- return json_success(
- json.dumps(
- [{"slice_id": slc.id, "slice_name": slc.slice_name} for slc in slices]
- )
- )
+ error = utils.error_msg_from_exception(ex)
+ status = None
+
+ result.append({"slice_id": slc.id, "error": error, "status": status})
Review comment:
@etr2460 @graceguo-supercat @villebro I was thinking about naming these `payload_error` and `payload_status`, though I guess the `Exception` error could be unrelated to the payload. Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9560: [fix] warm
up cache error handling
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9560: [fix] warm up cache error handling
URL: https://github.com/apache/incubator-superset/pull/9560#issuecomment-614878133
I think one of the more fundamental problems here is that `viz.py` is raising `Exception` for all problems, making it very difficult to distinguish between serious and less serious problems. In the interim this is probably a good solution, but going forward I hope we can replace generic Exceptions with more specific ones to distinguish between real serious problems and not so unexpected ones.
I don't have strong feelings about `error`/`payload_error` and `status`/`payload_status`. But my initial feeling is that is the `payload_` prefix adding context that would be lost without 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley merged pull request #9560: [fix]
warm up cache error handling
Posted by GitBox <gi...@apache.org>.
john-bodley merged pull request #9560: [fix] warm up cache error handling
URL: https://github.com/apache/incubator-superset/pull/9560
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9560: [fix] warm up cache error handling
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9560: [fix] warm up cache error handling
URL: https://github.com/apache/incubator-superset/pull/9560#discussion_r409872880
##########
File path: superset/views/core.py
##########
@@ -1707,15 +1709,16 @@ def warm_up_cache(self):
form_data=form_data,
force=True,
)
- obj.get_json()
+ payload = obj.get_payload()
+ error = payload["error"]
+ status = payload["status"]
except Exception as ex:
- logger.exception("Failed to warm up cache")
- return json_error_response(utils.error_msg_from_exception(ex))
- return json_success(
- json.dumps(
- [{"slice_id": slc.id, "slice_name": slc.slice_name} for slc in slices]
- )
- )
+ error = utils.error_msg_from_exception(ex)
+ status = None
+
+ result.append({"slice_id": slc.id, "error": error, "status": status})
Review comment:
this is the error and the status coming from the `get_viz` function, so perhaps this should be `viz_error` and `viz_status`? That seems clearer than payload_error/status since payload doesn't have a lot of meaning in the scope of a `warm_up_cache` function (it makes me ask "what payload?")
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org