Implement the shipper gRPC server

[rdner]

All the endpoints are actually functioning now.

This PR has no validation for incoming events, this will be addressed in a follow up PR #79.

Related to #34

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-08-09T07:08:54.343+0000

• Duration: 9 min 28 sec

Test stats 🧪

Test	Results
Failed	0
Passed	28
Skipped	0
Total	28

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[faec]

Apologies that I'm suggesting some fairly drastic changes, but I think it would allow a much simpler implementation.

[rdner]

@faec @cmacknz I propose we work on the server implementation in iterations and take more reasonable approach: if something does not work or works slow – we investigate and adjust the implementation. This PR is not supposed to be a final frozen version of the server and I see a lot of nitpicking going on in the review that's blocking the very first implementation. I think switching to sync.Cond from channel subscription was a reasonable comment but I can't see why the rest of the comments would block us from merging this.

[faec]

I can understand if you prefer another approach, but I don't think that removing several hundred lines of synchronization code is "nitpicking"... my point was rather that the first implementation should be smaller and simpler, since all the extra synchronization machinery isn't needed for any current use cases. I don't really think Craig's suggestions are nitpicking either, as some of the issues with the conditions and channels could let a single unresponsive client back up channels for the whole server...

[rdner]

@faec

I don't really think Craig's suggestions are nitpicking either, as some of the issues with the conditions and channels could let a single unresponsive client back up channels for the whole server...

That's exactly the reason why I addressed those concerns switching to sync.Cond, or you mean something else?

Okay, I understand your point and since it's a blocker from your perspective I'll address it once I'm back and then ask for another review. Just didn't want to block any related shipper work for almost 2 weeks.

[cmacknz]

Decision made that PublishEvents should block: #84

This can be changed in a follow up PR if it is more convenient.

[rdner]

@cmacknz @faec please have another look, everything should be addressed now.

[cmacknz]

Looks good, left a few minor comments that I don't think are blocking.

Thanks for addressing all the feedback!

[faec]

One minor concern, otherwise looks good thanks!