Skip to content

Conversation

@jayaseelan-james
Copy link
Contributor

@jayaseelan-james jayaseelan-james commented Jan 31, 2024

What does this Pull Request accomplish?

Adds get_foo_connection(s) wrapper APIs in the BaseReservation to accommodate the multiplexer info in the returning typed connection object.
AB#2623289

Why should this Pull Request be merged?

Now with the reserve API containing the multiplexer info. This PR extends the support by adding multiplexer data to the connection object.

What testing has been done?

Included automated tests that verify the functionalities of the newly added APIs.

@jayaseelan-james
Copy link
Contributor Author

Team, I would like to get your thoughts on the possible approaches to append multiplexer info to the connection object and how it can be retrieved.

Approach 1 [Recommended]: This PR contains changes for this approach

  • In addition to the existing get_nifoo_connection(s), add separate APIs to get connections that have multiplexer for each respective get_nifoo_connection(s) with the name get_nifoo_connection(s)_with_multiplexer.
  • ✔Pros:
    • Users get type hinting out of the box.
  • ❌Cons:
    • Users need to pass in the multiplexer session type.
    • Users need to use separate APIs to get connections depending on the presence of a multiplexer.

ℹ️ Note: By this approach, an error will be thrown for the following cases:

  • The user attempts to get a connection that has a multiplexer by passing in an incorrect multiplexer session type.
  • The user tries to get multiple connections and one or more connections have multiplexers of different session types other than the type specified in the parameter.

Approach 2:

  • Extend the TypedConnection structure so that the user can continue using the same API with the trade-off that they lose type hinting as the session info and sessions will be object type.
  • ✔Pros:
    • Users can use the same API to get connections regardless of the multiplexer's presence.
    • Users need not pass multiplexer session type as a parameter to the API.
  • ❌Cons:
    • Users don't get type hinting out of the box. It would be their responsibility to typecast the objects and use them accordingly.

Approach 3 [Not Recommended]:

  • Add type-specific properties in the Connection object. By this, a separate property will be added for each multiplexer type in the Connection object.
  • ✔Pros:
    • Users can use the same API to get connections regardless of the multiplexer's presence.
    • Users get type hinting out of the box.
  • ❌Cons:
    • Users must remember and use different properties for different multiplexer types.
    • This approach is not scalable as a property must be added each time when we support a new multiplexer type.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 31, 2024

Test Results

    25 files   -     5      25 suites   - 5   26m 9s ⏱️ - 9m 45s
   575 tests +   18     570 ✅ +   14      5 💤 + 4  0 ❌ ±0 
11 345 runs   - 2 345  10 260 ✅  - 2 360  1 085 💤 +15  0 ❌ ±0 

Results for commit ab07be9. ± Comparison against base commit b6b0ca5.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@dixonjoel dixonjoel left a comment

Choose a reason for hiding this comment

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

Approved for RFC and approach. Please reset me if you make this a regular PR.

@jayaseelan-james jayaseelan-james changed the title [RFC] nims: add get connection(s) wrapper API to accommodate mux info in the return value nims: add get connection(s) wrapper API to accommodate mux info in the return value Feb 2, 2024
@jayaseelan-james
Copy link
Contributor Author

@dixonjoel The current implementation represents the fair changes. I apologize for neglecting to update the PR title. 😞 I will address the initialize_multiplexer_sessions API additions in a separate PR.

Therefore, I'm resetting your vote. Sorry about the confusion.

@jayaseelan-james jayaseelan-james changed the title nims: add get connection(s) wrapper API to accommodate mux info in the return value nims: add get connection(s) with multiplexer APIs to accommodate mux info Feb 2, 2024
@jayaseelan-james
Copy link
Contributor Author

I verified all the tests locally using Python 3.9.13-win32. Therefore, I'm skipping the system tests (py32).

@jayaseelan-james jayaseelan-james merged commit eedc2e5 into main Feb 7, 2024
@jayaseelan-james jayaseelan-james deleted the users/jay/return-mux-data-from-get-connections branch February 7, 2024 05:52
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.

5 participants