Skip to content

RPC Queue#5

Merged
0xFA11 merged 12 commits intomasterfrom
rpcqueue
Feb 9, 2021
Merged

RPC Queue#5
0xFA11 merged 12 commits intomasterfrom
rpcqueue

Conversation

@NoelStephensUnity
Copy link
Copy Markdown
Member

@NoelStephensUnity NoelStephensUnity commented Dec 16, 2020

See linked RFC document →

First pass at getting an RFC in place for the RPC Queue stuff.

First pass at getting an RFC in place for the RPC Queue stuff.
@0xFA11
Copy link
Copy Markdown
Contributor

0xFA11 commented Dec 16, 2020

I will be trying to figure out how we could put our diagrams into markdown documents, so styling/formatting TBD :)

@0xFA11 0xFA11 changed the title RPC Queue Implementation RPC Queue Dec 20, 2020
Comment thread text/0000-rpc-queue.md Outdated
Comment thread text/0000-rpc-queue.md Outdated
Comment thread text/0000-rpc-queue.md Outdated
Comment on lines +45 to +48
# Drawbacks
[drawbacks]: #drawbacks

Why should we _not_ do this?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This section is meant for a very specific purpose as the template outlines: "Why we should not do this?"

One thing immediately pops-up in my mind is framework-layer vs transport-layer trade-off.
Currently, transport layer is not very helpful when it comes down to determining a proper network frame (with frame start and frame end).

However, we might hold onto this RFC and revisit RPC queuing after we figure out a better transport-layer interface abstraction and implementation (with queuing highly encouraged -or forced- via transport interface API).
If that happens, the feature this RFC suggest might simply become obsolete and transport-layer queuing would effectively replace both RPC queuing and also all other potential queues as well.

So my ask here is to talk a little bit more about this trade-off and outline these decisions by picturing our current state in the project, also mention potential future work coming up on the transport layer so that we can come back to this RFC at any point in the future, understand the thought-process behind this feature RFC and/or move towards a better transport-layer abstraction with the given context from this RFC.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The most recent version further addresses this area of the RFC.

Comment thread text/0000-rpc-queue.md Outdated
Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 left a comment

Choose a reason for hiding this comment

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

Please see my comments above.

Applied all updates to class and property names as well as related diagrams.
Added some additional information related to the drawbacks and future possibilities.
Fixed improper letter case for the inbound high level overview diagram.
Comment thread text/0000-rpc-queue.md Outdated
Comment thread text/0000-rpc-queue.md Outdated
Comment on lines +66 to +68
# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
Since there are no current discussions on batching specific packet types together at the transport layer (not recommended) and while Unet provides the ability to switch from immediate to queued mode there were other framework related design factors taken into condiseration (i.e. batched RPCs, invocation at different update stages, etc.) that made this the current best path to take.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not comment (at least briefly) what we considered and why this approach suits our needs for now? also, an alternative might be to have QueueSend()/FlushSend()/etc. APIs on transport itself and expect it to happen under the hood for us, implemented by transport implementors. I'd like to see at least transport-level queuing vs framework-level queuing being mentioned here (especially since we have plans in the future in that direction already, why not note why this particular RFC is more useful than others today but potentially not tomorrow?).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added a blurb on this in the most recent refactor. I held back on going into any specific details as we still need some additional clarity regarding what we are going to expect from the default transport used with MLAPI (i.e. what the default Unity transport will end up being) and any changes to the framework transport interface that may be needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The most recent version should address this area further.

Comment thread text/0000-rpc-queue.md Outdated
Comment thread text/0000-rpc-queue.md
Added additional language based on feedback from @MFatihMAR.
NoelStephensUnity and others added 3 commits February 3, 2021 11:02
This addresses some comments Fatih had made about various areas within this RFC.
Removed the UTP blurb text.
@0xFA11 0xFA11 merged commit 8b244f1 into master Feb 9, 2021
@LukeStampfli LukeStampfli added final-comment-period RFC is in final comment period accepted RFC has been accepted and removed final-comment-period RFC is in final comment period labels Feb 15, 2021
@0xFA11 0xFA11 deleted the rpcqueue branch March 9, 2021 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted RFC has been accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants