-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
Updated Redis Connections to use the correct credentials #667
Updated Redis Connections to use the correct credentials #667
Conversation
…ecifically for their purpose in plainer language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few minor comments, then one thing that I think needs to be fixed.
...n/services/evaluationservice/dmod/evaluationservice/evaluation_service/consumers/listener.py
Outdated
Show resolved
Hide resolved
# The passed message may be a wrapper if it doesn't bear an event, but DOES have a 'data' member. | ||
# If that's the case, use its data member instead | ||
while is_message_wrapper(message): | ||
message = message['data'] | ||
message: typing.Dict[str, typing.Any] = message['data'] | ||
request_id: typing.Optional[str] = get_request_id(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal, but is there a reason why this setting of request_id
needs to be inside the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redefinition of typing.Optional[str]
here is just from copying and pasting, but the update of request_id
is here to capture any possible redeclaration of request_id
from within the message contents. The field is used for tracking async responses that may need to be acted upon, so if an optional request_id
is passed through there, there'll be something on the other end listening for a response with it attached. Gotta make sure it has the most specific value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the request_id
ever be allowed to change? Seems like that could be an invariant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the message artifact or the variable in the function? Messages may come in looking something like:
{
"event": "search",
"type": "send_message",
"time": "6/18/2024, 12:31:09 PM",
"data": [
{
"identifier": 1,
"author": "christopher.tubbs",
"name": "Standard Test Evaluation",
"description": "The basic evaluation used for all testing purposes\n "
},
{
"identifier": 2,
"author": "christopher.tubbs",
"name": "Test Evaluation",
"description": "A sample evaluation to be run locally"
}
],
"request_id": "390l1uz74uf-n238l24383o"
}
or they may come in with the format:
{
"event": "forward",
"type": "send_message",
"time": "6/18/2024, 12:31:09 PM",
"data": {
"event": "search",
"type": "send_message",
"time": "6/18/2024, 12:31:09 PM",
"data": [
{
"identifier": 1,
"author": "christopher.tubbs",
"name": "Standard Test Evaluation",
"description": "The basic evaluation used for all testing purposes\n "
},
{
"identifier": 2,
"author": "christopher.tubbs",
"name": "Test Evaluation",
"description": "A sample evaluation to be run locally"
}
],
"request_id": "390l1uz74uf-n238l24383o"
},
"request_id": "8sdfu57aOa5-8s5SF85a846"
}
In both cases, the request_id
of interest is "390l1uz74uf-n238l24383o". While the second example is far less common than the first, the inner most request_id
value is what is of interest, not the outer.
Furthermore, that is the case with messages sent from the client that need to keep track of the responses to individual requests. Messages that do not need to keep track of an accompanying response may look something like:
{
"event": "connect",
"type": "send_message",
"time": "6/18/2024, 12:31:02 PM",
"data": {
"message": "Connection Accepted"
},
"request_id": null
}
Messages originating from a client will most likely have a request_id
on it, but it isn't required. It is just a tool that a client may take advantage of.
|
||
while is_message_wrapper(deserialized_message): | ||
# This is only considered a message wrapper if it is a dict; linters may think this could be a string, | ||
# but it will always be a dict here | ||
deserialized_message = deserialized_message['data'] | ||
|
||
request_id = get_request_id(deserialized_message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, why do this inside the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above; the definition of deserialized_message
is being drilled into, so any possible definition of request_id
within needs to be captured.
...n/services/evaluationservice/dmod/evaluationservice/evaluation_service/consumers/listener.py
Show resolved
Hide resolved
python/services/evaluationservice/dmod/evaluationservice/utilities/communication.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building on @robertbartel's review, I had a few questions. Nothing major.
...n/services/evaluationservice/dmod/evaluationservice/evaluation_service/consumers/listener.py
Show resolved
Hide resolved
...n/services/evaluationservice/dmod/evaluationservice/evaluation_service/consumers/listener.py
Outdated
Show resolved
Hide resolved
# The passed message may be a wrapper if it doesn't bear an event, but DOES have a 'data' member. | ||
# If that's the case, use its data member instead | ||
while is_message_wrapper(message): | ||
message = message['data'] | ||
message: typing.Dict[str, typing.Any] = message['data'] | ||
request_id: typing.Optional[str] = get_request_id(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the request_id
ever be allowed to change? Seems like that could be an invariant.
...n/services/evaluationservice/dmod/evaluationservice/evaluation_service/consumers/listener.py
Show resolved
Hide resolved
python/services/evaluationservice/dmod/evaluationservice/service/application_values.py
Outdated
Show resolved
Hide resolved
…ssue where the function that gets a connection to the redis runner was using the baseline redis credentials, added documentation and clarifications to the websocket listener, and made it so that `get_request_id` doesn't rely on external state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for making the changes!
Requested changes have been addressed.
Actions taken previously were scattered between using a default redis password and redis hosts and ports for a defunct RQ configuration. That has been changed so that the elements connected to channels are connected to credentials for channels, elements connected to the runner are connected to the credentials for the runner, and elements connected to just redis by default are connected to the right credentials.
closes #649