Skip to content

RED-124787 - add batch_execute #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 25, 2024
Merged

RED-124787 - add batch_execute #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2467427
add batch_execute
hassankh148 Apr 17, 2024
2f9f1cc
use python 3.8
hassankh148 Apr 24, 2024
f4612d7
CR
hassankh148 Apr 24, 2024
065cfbe
CR
hassankh148 Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM python:3.6.7
FROM python:3.8

ENV FLASK_APP app.py
ENV APP_SETTINGS settings.cfg
Expand Down
35 changes: 30 additions & 5 deletions app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,33 +88,58 @@ def return_code(self):
return self._return_code


@app.route('/execute', methods=['POST'])
def execute():
def _execute(command: str):
success = False
req = request.get_json()
try:
conn = get_conn_through_sentinel()
response = conn.execute_command(*req['command'].split())
response = conn.execute_command(*command.split())
success = True
except (redis.exceptions.ConnectionError, redis.exceptions.ResponseError):
try:
reload_username_password_from_file_system_if_needed(app)
conn = get_conn_through_sentinel()
response = conn.execute_command(*req['command'].split())
response = conn.execute_command(*command.split())
success = True
except Exception as err:
response = 'Exception: cannot connect. %s' % str(err)
app.logger.exception("execute err")
except Exception as err:
response = 'Exception: %s' % str(err)
app.logger.exception("execute err")
return response, success
Copy link

@oshribr oshribr Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://github.com/RedisLabs/redis-enterprise-operator/blob/main/integration_tests/src/webcli/redis_web_cli_client.py#L52

Copy link
Contributor Author

@hassankh148 hassankh148 Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea and I agree, but out of scope of this ticket, especially since there are other calls that already follow this convention.



@app.route('/execute', methods=['POST'])
def execute():
req = request.get_json()
response, success = _execute(req['command'])

return jsonify({
'response': response,
'success': success
})


@app.route('/batch_execute', methods=['POST'])
def batch_execute():
Copy link

@zcahana zcahana Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hassankh148 hassankh148 Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

all_succeeded = True
Copy link

@oshribr oshribr Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hassankh148 hassankh148 Apr 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

responses = []
req = request.get_json()
commands = req['commands']
for command in commands:
response, success = _execute(command)
responses.append({
'response': response,
'success': success,
})
if not success:
all_succeeded = False
return jsonify({
'response': responses,
'success': all_succeeded
})


def reload_username_password_from_file_system_if_needed(app):
# It may be that the dynamic password was changed since the config was set
if should_read_from_file_system():
Expand Down