Use Chariott

[ashbeitz]

No description provided.

[github-actions]

Alex Recommends Report

Alex recommends the following language changes, but Alex is a regular expression based algorithm, so take them with a grain of salt.

✨ 🚀 ✨ Nothing to Report ✨ 🚀 ✨

[jorchiu]

Please take a look at my comments.

[ladatz]

Please consider my comments

[wilyle]

this is more related to chariott than this PR, but this interface seems very strange to me. Why does request.into_inner().intent have a property also named intent? also, the use of and_then looks awkward here, though I don't know if there's a better way to do this right now.

tagging @ladatz since this is related to chariott APIs

[wilyle]

edit: I've read a bit more and apparently and_then is the correct idiomatic way to achieve what I wrote below rather than .map(...).flatten(), so I don't think we should use this suggestion. I still don't really like using and_then because I think it looks weird so we should still consider the other suggestion in the next comment

---

for a more intuitive way of doing this, we can consider this slightly more verbose approach:

let fulfillment = match request
    .into_inner()
    .intent
    .map(|i| i.intent)
    .flatten()
    .ok_or_else(...)? {...}

I think using the map function better illustrates the intent here (no pun intended), and flatten() is necessary to go from the resulting Option<Option<T>> to just Option<T>

[wilyle]

Alternatively we could call ok_or_else twice in the call chain which is probably clearer:

let fulfillment = match request
    .into_inner()
    .intent
    .ok_or_else(...)?
    .intent
    .ok_or_else(...)? {...}

[devkelley]

To answer why it looks so awkward, I believe its because of the use of oneof in the common proto file in chariott. The FulfillRequest looks like this in runtime.proto

And it references this structure in common.proto. The 'outer' intent is mapped to the message, while the 'inner' intent is mapped to the oneof kind:

[wilyle]

would it make sense to change the intent API to look like this:

message Intent {
    oneof value {
        ...
    }
}

Then the API to access it would be (ignoring Option syntax) request.intent.value

Also, do you have any comments on my suggestions for how to access the value here in the code with all of the extra syntax? The current code is derived from the chariott samples so these suggestions would propagate to there as well

[devkelley]

For the API, I agree that a change in the protobuf structure would be valuable to make things more readable. In terms of the suggestions, I would say that the first one looks a little more clear to me, however its not significantly more clear than what is already there (my opinion). I would say that since this is intended to be changed with chariott redesign that its okay to leave as is.

[ladatz]

I agree that request -> intent -> value looks a lot better than what we have now. Of Will's suggestions above, I think I like the multiple ok_or_else option, but I need a little more investigation myself. For this Ibeji PR sepcifically, I also agree with Devin that we could leave it as-is in Ibeji for now. The following Issue (for chariott service discovery simplification) is currently being worked on, and from Ibeji's perspective, it will remove the need for this code at all: eclipse-chariott/chariott#134. This is a good discussion to keep in mind as we modify Chariott's APIs moving forward, though.

[ashbeitz]

Let’s not address this concern for now, as it should either go away or be addressed by Chariott's Simplify Service Discovery work and follow-on component specific refactoring.

[wilyle]

there is probably a similar approach here as suggested for the intent API to make this easier to understand

[ashbeitz]

Let’s not address this concern for now, as it should either go away or be addressed by Chariott's Simplify Service Discovery work and follow-on component specific refactoring.

[wilyle]

instead of a match statement here you could consider using ok_or, though that would not let you include the info statement on line 103 easily if you want to have that

[ashbeitz]

We chatted about this. I want to leave the logging in, so it's fine as is.