Skip to content

Conversation

spyrostz
Copy link
Member

@spyrostz spyrostz commented Oct 19, 2022

…der to be able to connect to a simulation using a specific id even when running via Redis. Ignored messages from areas that are not registered to this sdk execution.

Reason for the proposed changes

Please describe what we want to achieve and why.

Proposed changes

INTEGRATION_TESTS_BRANCH=master

GSY_E_TARGET_BRANCH=fix_external_load_forecast_strategy

GSY_FRAMEWORK_BRANCH=master

…der to be able to connect to a simulation using a specific id even when running via Redis. Ignored messages from areas that are not registered to this sdk execution.
@spyrostz spyrostz requested a review from a team October 19, 2022 09:23
…ll simulations but only to the one defined by simulation_id (which is empty string for the case of tests).
mrsaemir
mrsaemir previously approved these changes Oct 19, 2022
Copy link
Contributor

@mrsaemir mrsaemir left a comment

Choose a reason for hiding this comment

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

LGTM with 1 suggestion. Thanks a lot.

"""Test side effects due to the methods called with _connect_and_subscribe().
-> Side effects of _subscribe_to_response_channels private method
"""
aggregator = RedisAggregator(aggregator_name=TEST_AGGREGATOR_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to set the simulation_id on the aggregatator to also test the part where the init function is reading simulation_id from env variables. That can potentially make your test even more robust. Thanks a lot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

channel_dict_2 = {f"external-aggregator/*/{aggregator.aggregator_uuid}/events"
channel_dict_2 = {f"external-aggregator//{aggregator.aggregator_uuid}/events"
f"/all": aggregator._events_callback_dict,
f"external-aggregator/*/{aggregator.aggregator_uuid}/response"
Copy link
Member

Choose a reason for hiding this comment

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

So this * is not needed any more in the subscribe pattern?

Copy link
Member Author

@spyrostz spyrostz Oct 20, 2022

Choose a reason for hiding this comment

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

It was never needed actually. This * represents that the aggregator subsribes to all responses from all simulations. Up until now it was fine because we were only running one simulation, however it could not work correctly if we run more than one simulation at the same time. To be more detailed, this pattern will match both following channels:

external-aggregator/sim1/aggregator1/response
external-aggregator/sim2/aggregator1/response

Of course, we never fell upon this case because we are protected by the uuid of the aggregator (which is unique, therefore the channel name is also unique - in other words, there is never the case that the uuid e.g. aggregator1 will be generated for both simulations). Nevertheless, does not hurt to make it more detailed only listen to channels that are destined for one specific simulation. Hence I removed this.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the verbose answer! I still wonder though: What does the absence of the * mean?
Doesn't it also mean:

the aggregator subsribes to all responses from all simulations

?

Copy link
Member Author

@spyrostz spyrostz Dec 14, 2022

Choose a reason for hiding this comment

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

Sorry took a long time to check it out. Actually the * is not absent, it is just replaced with the actual simulation id, therefore each SDK instance will connect only to one simulation with this specified ID. What you describe here :

the aggregator subsribes to all responses from all simulations

was the former behavior, because the * in the channel name subscribed to all responses from all simulations. Now that * is replaced with the actual simulation id , the aggregator subscribes to all responses from one specific simulation.

Ah, and if your question was actually asking what would this mean external-aggregator//aggregator1/response , this would match all responses sent in that exact channel, since no wildcard is part of the channel.

Copy link
Member

@hannesdiedrich hannesdiedrich left a comment

Choose a reason for hiding this comment

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

Once Amirhosseins' comment is dealt with, LGTM

…of unit tests, in order to test that the simulation id set by the variable is correctly passed to the channel name. Modified test that does not explicitly test the simulation_id assignment to use the aggregator._simulation_id variable in order to be future-proof.
Copy link
Contributor

@mrsaemir mrsaemir left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants