Skip to content
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

Dynamic Traits Cookbook #28

Merged
merged 5 commits into from
Mar 22, 2022
Merged

Dynamic Traits Cookbook #28

merged 5 commits into from
Mar 22, 2022

Conversation

AdityaRandive
Copy link
Contributor

This commit illusrates Dynamic Traits w.r.t. the Agents, respective properties
and the shared lanes.

Author: Aditya R
Reviewers: Ajay G, Rohit B
Test: Manual
Issue: #24

This commit illusrates Dynamic Traits w.r.t. the Agents, respective properties
and the shared lanes.

Author: Aditya R
Reviewers: Ajay G, Rohit B
Test: Manual
Issue: #24
Copy link
Member

@ajay-gov ajay-gov left a comment

Choose a reason for hiding this comment

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

The dynamic traits look good from a code perspective. Couple of suggestions

  • It might good to have the logic of determining what trait to load based on the data passed into a lane, say config. You could call that with space.command and the payload can be a Value with a type key which may be "Water" or "Juice" . The callback function on the config lane could load the appropriate trait based on the value of the type. That way you can remove the props in the recon file for /liquid/:id

  • Just the sharedInfo lane should be fine, not sure we need another `shared lane?

  • Minor thing, maybe use proper agent names instead of random ints as ids. /liquid/distilled, /liquid/applesay. Also it might be best to have/liquid/sparklingand/liquid/orange` for the static traits to be consistent. What do you think?

Copy link
Member

@brohitbrose brohitbrose left a comment

Choose a reason for hiding this comment

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

  • Checkstyle fixes
  • LiquidAgent.java: didStart logs the word "region", which doesn't make sense for this app -- I'm guessing this is an artifact from cellular

@AdityaRandive
Copy link
Contributor Author

AdityaRandive commented Mar 16, 2022

The dynamic traits look good from a code perspective. Couple of suggestions

  • It might good to have the logic of determining what trait to load based on the data passed into a lane, say config. You could call that with space.command and the payload can be a Value with a type key which may be "Water" or "Juice" . The callback function on the config lane could load the appropriate trait based on the value of the type. That way you can remove the props in the recon file for /liquid/:id
  • Just the sharedInfo lane should be fine, not sure we need another `shared lane?
  • Minor thing, maybe use proper agent names instead of random ints as ids. /liquid/distilled, /liquid/applesay. Also it might be best to have/liquid/sparklingand/liquid/orange` for the static traits to be consistent. What do you think?

Addressed (1) and (3). For (2) - cleaner lane sharing observed for static and dynamic, hence the two.

Copy link
Member

@brohitbrose brohitbrose left a comment

Choose a reason for hiding this comment

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

Looks good other than the aforementioned comment.

traits/src/main/resources/server.recon Outdated Show resolved Hide resolved
@ajay-gov
Copy link
Member

@AdityaRandive The general pattern would be for the traits (i.e. Juice or Water to have logic that sets the shared lane which then gets reflected in the base agent i.e Liquid. So I think it will be good to move the calls to set the lanes to the Juice and Water agents.

Also having two lanes, one for sharing info of static traits and one for sharing info of dynamic traits, is a bit confusing. The main idea here is to show that you can have the same lane and use it across static and dynamic traits and share "info". That way downstream clients that access data from these agents can just subscribe to one lane i.e sharedInfo lane. Otherwise they will have to subscribe to two lanes which seems a bit more tedious.

@AdityaRandive
Copy link
Contributor Author

@AdityaRandive The general pattern would be for the traits (i.e. Juice or Water to have logic that sets the shared lane which then gets reflected in the base agent i.e Liquid. So I think it will be good to move the calls to set the lanes to the Juice and Water agents.

Also having two lanes, one for sharing info of static traits and one for sharing info of dynamic traits, is a bit confusing. The main idea here is to show that you can have the same lane and use it across static and dynamic traits and share "info". That way downstream clients that access data from these agents can just subscribe to one lane i.e sharedInfo lane. Otherwise they will have to subscribe to two lanes which seems a bit more tedious.

ACK

@AdityaRandive AdityaRandive merged commit 23f81f2 into master Mar 22, 2022
@AdityaRandive AdityaRandive deleted the dynamic-traits branch March 22, 2022 03:21
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