Javadocs for merlin.protocol

[Twisol]

• Tickets addressed: N/A
• Review: By commit
• Merge strategy: Merge (no squash)

Description

This PR contains a bunch of actual Javadocs for the merlin.protocol package. It's still in progress, but this is the kind of thing that can be done incrementally!

Also, a fair amount of "documentation" came in #318, which reorganized and renamed many of the entities in merlin.protocol to give a better baseline of self-documenting code. That isn't to say that prose documentation isn't needed (heavens, no!) -- only that readers of the prose documentation herein will greatly benefit from the much tighter naming and organization prepared in #318.

As usual, this PR also comes with a (small) prefix of refactors that came up during the main work. (Really, #318 was the prefix for this work, but it was separated out because of how much there was to it -- and the bug it ultimately fixed.) Here, I've cleaned up some Gradle dependencies in merlin-sdk and removed a type parameter that no longer matters.

Verification

It all compiles and runs; the changes made in this PR are either documentation-only or do not impinge on business logic. (For instance, the type parameter removal is the kind of thing where, if it's wrong, the system should simply not compile. Type checking is a form of testing!)

Documentation

Lots of javadocs were added, but nothing existing needs to change.

Future work

Document all the things!

[github-actions]

Automatically approved due to detection of the following labels: documentation

[github-actions]

Automatically approved due to detection of the following labels: documentation

[mattdailis]

I was going to put this comment on #318 , but it may be more appropriate here since you're in the process of explaining all the things 😉 (Let me know if you think otherwise and I can re-post on the other PR)

My first question was regarding Task vs TaskFactory - previously, ActivityActions would call spawn(DirectiveId, Task), and now they call spawn(TaskFactory). I have no concerns about this, but I'm not sure I appreciate the nuance - could you help me understand the reasoning here?

Just thinking out loud, the one observable difference I can think of is that previously calling spawn(new UserDefinedActivity()) during Mission model initialization would result in an IllegalStateException, whereas now it will spawn that activity's effect model as a daemon task...? 🤔

Also - I remember we went back and forth on how to consolidate the two Scheduler::spawn methods (one of which was for directives, one of which was for anonymous tasks). Could you comment on how you were able to consolidate them both into spawn(TaskFactory)?

[Twisol]

I was going to put this comment on #318 , but it may be more appropriate here since you're in the process of explaining all the things 😉 (Let me know if you think otherwise and I can re-post on the other PR)

Here is fine!

My first question was regarding Task vs TaskFactory - previously, ActivityActions would call spawn(DirectiveId, Task), and now they call spawn(TaskFactory). I have no concerns about this, but I'm not sure I appreciate the nuance - could you help me understand the reasoning here?

Just thinking out loud, the one observable difference I can think of is that previously calling spawn(new UserDefinedActivity()) during Mission model initialization would result in an IllegalStateException, whereas now it will spawn that activity's effect model as a daemon task...? 🤔

Well, the difference between Task and TaskFactory is not so great; if you have a TaskFactory, you can always get a Task. The big change was removing DirectiveTypeId (or whatever it was called), which was how we previously associated a directive type to its input and output serialization routines for sim result extraction. The simulation engine doesn't use the DirectiveTypeId anymore, due to an even earlier PR that identifies directive start/stop events by their registered topic name (which is something a downstream client in principle ought to be doing instead, but I digress).

The difference between a daemon task and regular task delegation is simply when spawn is called, not how it is called. When you spawn a task during initialization, there is no true parent task. Put differently, model construction must complete before simulation can proceed, so you can't do some things tasks normally can (and you can do some things that tasks normally cannot, like cell allocation).

Also, using TaskFactory everywhere instead of Task made it easier to consolidate task creation on the engine side, where the root model is always easily available. Using the Task version of spawn in ActivityActions#spawn (and friends) required us to also fetch the root model (so we could use its stored executor for any spawned ThreadedTasks), which simply isn't necessary anymore. This is what let me get rid of the second Scoped variable entirely in #318.

Also - I remember we went back and forth on how to consolidate the two Scheduler::spawn methods (one of which was for directives, one of which was for anonymous tasks). Could you comment on how you were able to consolidate them both into spawn(TaskFactory)?

I think we had debated adding a special DirectiveTypeId for anonymous tasks, so that we could unify everything into the two-argument form. Since we have no need of DirectiveTypeId at all anymore, everthing naturally unified into the single-argument form.

Does that answer your questions? 🤔

[Twisol]

Also, one of the original motivations for TaskFactory was that model construction might be expensive; TaskFactory lets us reuse the same constructed model across multiple simulations, so long as they're configured the same, since we can create fresh daemon tasks for each simulation.

It turns out that TaskFactory is simply more convenient than Task in other cases, too, so it's started to take over.

I'm really considering making the CallingTask variant of TaskStatus take a TaskFactory too, for the same reason. (Also it makes it less easy to confuse the child for the parent's continuation.)

[Twisol]

Huh! Looks like I never made Scheduler#spawn take a TaskFactory. We're currently threading the executor through ReplayingTask even though that class doesn't use it, just so that we can propagate it to spawned tasks -- I think we can remove that flow entirely by switching to TaskFactory in Scheduler#spawn and TaskStatus#calling. Let's find out!

[Twisol]

Well, that escalated quickly. #382 is the culmination of moving more things to using TaskFactory. It is very nice.

[Twisol]

Could we get one more approval on these Javadocs? 👀