RED-124787 - add batch_execute #23
Conversation
|
|
||
| @app.route('/batch_execute', methods=['POST']) | ||
| def batch_execute(): | ||
| all_succeeded = True |
There was a problem hiding this comment.
Instead of returning a boolean, it's better to count successes or failures.
However, since all responses are returned in a dictionary, we don't need this field.
There was a problem hiding this comment.
I am following the convention used in all others and already used in single execute command. here success=True indicates that all commands have succeeded, and False otherwise.
app.py
Outdated
|
|
||
| @app.route('/execute', methods=['POST']) | ||
| def execute(): | ||
| def _execute(request_as_string: str): |
There was a problem hiding this comment.
| def _execute(request_as_string: str): | |
| def _execute(command: str, *arg: str): |
There was a problem hiding this comment.
conn.execute_command() receives all its arguments as *args including the command itself, so no point in splitting the given full command as command+args.
There was a problem hiding this comment.
I agree with both of you so propose a middleground:
def _execute(command: str):commandis the term we use all over the place- The
as_stringsuffix is redundant - No need for separate command and args
| except Exception as err: | ||
| response = 'Exception: %s' % str(err) | ||
| app.logger.exception("execute err") | ||
| return response, success |
There was a problem hiding this comment.
It might be a good idea to change the return value of success to an error message as a string. In this case, an empty string would indicate success, much like the way Golang handles errors.
Note: by doing so, you will need to fix the web_cli.
There was a problem hiding this comment.
good idea and I agree, but out of scope of this ticket, especially since there are other calls that already follow this convention.
zcahana
left a comment
zcahana
left a comment
There was a problem hiding this comment.
Approved with one minor naming suggestion.
app.py
Outdated
|
|
||
| @app.route('/execute', methods=['POST']) | ||
| def execute(): | ||
| def _execute(request_as_string: str): |
There was a problem hiding this comment.
I agree with both of you so propose a middleground:
def _execute(command: str):commandis the term we use all over the place- The
as_stringsuffix is redundant - No need for separate command and args
|
|
||
|
|
||
| @app.route('/batch_execute', methods=['POST']) | ||
| def batch_execute(): |
There was a problem hiding this comment.
I'm wondering whether this synchronous batch execution model would solve the connection problems we tried to avoid with this change. I thought we'll do an asynchronous batch request and then collect the results. However, let's give it a chance.
There was a problem hiding this comment.
this synchronous approach was suggested because the flakiness occurred in the integration tests' part, the fix is two-fold, having the batch_execute and reverting redis-webcli-client to use K8s proxy to service. this way we use the more "stable" option and at the same time not overload K8s proxy with 50 requests for each read/write during test_upgrade
app.py
Outdated
| @app.route('/batch_execute', methods=['POST']) | ||
| def batch_execute(): | ||
| all_succeeded = True | ||
| responses = dict() |
There was a problem hiding this comment.
Actually, a dict is not ideal here because there could be multiple input commands which are similar.
I'd just use a list of responses, with their index matching the index of the input command.
No description provided.