Conversation
| string message = 2; | ||
| int32 exit_code = 3; | ||
| string details = 4; | ||
| } |
There was a problem hiding this comment.
We need comments here. All these fields look like a bunch of redundant information.
There was a problem hiding this comment.
Ok I will add more comments to explain
There was a problem hiding this comment.
This is marked as "resolved" but I don't see comments...
rneswold
left a comment
rneswold
left a comment
There was a problem hiding this comment.
I'm not a fan of commenting-out code and committing it. The git history will tell you what was changed.
Are those commented-out fields going to be re-introduced at a later time? If not, just delete them. We don't have a production-ready FaaS system, so I'd renumber the fields, if you delete the commented ones.
| rpc UpdateMessageQueueTrigger(UpdateMQTriggerRequest) returns (GenericResponse); | ||
| rpc DeleteMessageQueueTrigger(DeleteMQTriggerRequest) returns (GenericResponse); | ||
|
|
||
| // Lust of functions and triggers |
There was a problem hiding this comment.
I have added this features to let user know how many functions and trigger are there
There was a problem hiding this comment.
I was giggling at your code comment "Lust of functions". 😄
|
I will remove the commented part. |
beauremus
left a comment
beauremus
left a comment
There was a problem hiding this comment.
This looks good, but needs polishing. If you like my suggestions, then I'm ready to approve.
| string message = 2; | ||
| int32 exit_code = 3; | ||
| string details = 4; | ||
| } |
There was a problem hiding this comment.
This is marked as "resolved" but I don't see comments...
|
|
||
| package services.faas; | ||
|
|
||
| // high-level RPC to manage Fission resources |
There was a problem hiding this comment.
It's not clear what this comment pertains to. Either remove it or move it closer to the relevant code.
There was a problem hiding this comment.
AH! If it's a summary of this entire definition, then maybe say more about the approach.
|
|
||
| // By default all functions will have HTTP trigger | ||
| // HTTP trigger operation to expose function via Fission router | ||
| //rpc CreateHTTPTrigger(CreateHTTPTriggerRequest) returns (GenericResponse); |
There was a problem hiding this comment.
It's not clear why these three rpc calls exist when we have the ones below. If the latter rpcs replace these, then remove these. No commented code.
| } | ||
|
|
||
| // CreateHTTPTrigger | ||
| //message CreateHTTPTriggerRequest { |
There was a problem hiding this comment.
If this isn't used, remove it. No commented code.
| //} | ||
|
|
||
| // DeleteHTTPTrigger | ||
| //message DeleteHTTPTriggerRequest { |
There was a problem hiding this comment.
If this isn't used, remove it. No commented code.
| } | ||
|
|
||
| // UpdateFunction | ||
| message UpdateFunctionRequest { |
There was a problem hiding this comment.
This is identical to CreateFunctionRequest. This is fragile because if one changes, you'd expect the other to change, and a future developer may not recognize that. I recommend merging these into UpsertFunctionRequest and reusing this where appropriate.
| //rpc UpdateHTTPTrigger(UpdateHTTPTriggerRequest) returns (GenericResponse); | ||
| //rpc DeleteHTTPTrigger(DeleteHTTPTriggerRequest) returns (GenericResponse); | ||
|
|
||
| // Create timer trigger (cron-like spec) |
There was a problem hiding this comment.
What motivates these separate Trigggerrpcs? It's intuitive to me that these structures should be part of the Upsert. What do you think? I think this, along with my other recommendations, could really simplify this protocol.
| string error_mq = 5; | ||
| string maxtries = 6; | ||
| string cooldowntime = 7; | ||
| string poolingtime = 8; |
There was a problem hiding this comment.
Seems like these fields could be integers (6, 7, and 8.)
| string error_mq = 5; | ||
| string maxtries = 6; | ||
| string cooldowntime = 7; | ||
| string poolingtime = 8; |
There was a problem hiding this comment.
Fields 6, 7, and 8 should be integers, right?
3 participants
gRPC for fission