Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added gRPC logging stream server #147

Merged
merged 26 commits into from
Mar 3, 2022
Merged

Added gRPC logging stream server #147

merged 26 commits into from
Mar 3, 2022

Conversation

applejag
Copy link
Contributor

@applejag applejag commented Feb 21, 2022

Summary

  • Added api/wharfapi/v1/builds.proto
  • Added Protobuf compiling to Makefile
  • Added dependency on Go gRPC libraries
  • Added multiplexing of gRPC and Gin servers on same port via new dependency soheilhy/cmux
  • Added gRPC server implementation in grpc.go
  • Added database log batch injection SQL for sqlite & postgres.
  • Moved all HTTP server stuff to http.go

Motivation

The SQL code is what mostly stands out in this PR IMO. What I wanted to do was:

  • In a single SQL query do batch insertion of multiple log lines.
  • Skip insertion of logs that targets non-existing builds to not cause foreign key violation.
  • Get a list back of the logs that were not skipped, so we can publish them via server-sent events (SSE) to any web clients.

In Sqlite this was simple, as you basically do INSERT OR IGNORE INTO instead of just INSERT INTO, while Postgres does not have such a feature, but I found a solution that instead made use of a JOIN statement inside the INSERT.

Please look at the test files for the best overview of what the SQL actually looks like.

The Sqlite function isn't really working as we do not have foreign key constraints enabled. However, this is planned in #145.

Protobuf versioning

I've added the major version to both the path in api/wharfapi/v5/builds.proto as well as the Protobuf package name wharf.api.v5.

My reasoning here is that it should be the same as the wharf-api version, for simplicity sake. But to be able to keep backwards compatiblity, when jumping to v6 the wharf-api needs to keep supporting v5. In other words, when bumping to v6, we will have the files:

  • api/wharfapi/v5/*.proto
  • api/wharfapi/v6/*.proto

For v7, we will have the files:

  • api/wharfapi/v6/*.proto
  • api/wharfapi/v7/*.proto

Up for discussion: Should the gRPC protobuf API have a separate versioning? It could, but I think that'd be adding excess confusion.

It does however make it so if we have a breaking change in the HTTP REST API then we also have to bump the gRPC API. Perhaps that's a good thing, but I'm not sure. Input on this is well appreciated.


Closes #130

@applejag applejag added the enhancement New feature or request label Feb 21, 2022
@applejag applejag self-assigned this Feb 21, 2022
@applejag applejag mentioned this pull request Feb 21, 2022
@applejag applejag force-pushed the feature/grpc-server branch 2 times, most recently from 3b292a9 to 369c022 Compare February 21, 2022 15:17
@applejag applejag force-pushed the feature/grpc-server branch from 369c022 to 5c18011 Compare February 24, 2022 15:10
@applejag applejag marked this pull request as ready for review February 24, 2022 15:10
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Good stuff 👍🏻

Nice to see how simple the multiplexing usage is o:

api/wharfapi/v5/builds.proto Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@applejag applejag requested a review from Alexamakans February 25, 2022 09:04
go.mod Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
@applejag applejag merged commit ae9081c into master Mar 3, 2022
@applejag applejag deleted the feature/grpc-server branch March 3, 2022 14:50
@applejag applejag mentioned this pull request Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC server for streaming logs insertions
2 participants