Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Dynamic control API exposed via gRPC (#32) #33

Merged
merged 788 commits into from
Oct 25, 2019

Conversation

evdokimovs
Copy link
Contributor

@evdokimovs evdokimovs commented Jul 4, 2019

Dynamic control API exposed via gRPC

Resolves #32

Part of 0.2.0 Roadmap #27

Synopsis

Need implement dynamic control API exposed via gRPC. In this PR will be implemented only Create, Get and Delete methods. Apply is not part of this PR and will be implemented in the future.

Solution

  • 1. Generate DTOs from this gRPC spec
  • 2. Implement gRPC server by grpcio crate
  • 3. Parse protobuf into interior structs
  • 4. Implement method Create
    • 4.1 Room
    • 4.2 Member
    • 4.3 Endpoint
  • 5. Implement method Get
    • 5.1 Room
    • 5.2 Member
    • 5.3 Endpoint
  • 6. Implement method Delete
    • 6.1 Room
    • 6.2 Member
    • 6.3 Endpoint
  • 7. Properly return errors
    • 7.1 Create
    • 7.2 Get
    • 7.3 Delete

Checklist

  • Created PR:
    • In draft mode
    • Name contains WIP: prefix
    • Name contains issue reference
    • Has k:: labels applied
    • Has assignee
  • Documentation is updated (if required)
  • Tests are updated (if required)
  • Changes conform code style
  • CHANGELOG entry is added (if required)
  • FCM (final commit message) is posted
    • and approved
  • Review is completed and changes are approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • WIP: prefix is removed
    • All temporary labels are removed

@evdokimovs evdokimovs added feature New feature or request k::api Related to API (application interface) labels Jul 4, 2019
@evdokimovs evdokimovs added this to the 0.2.0 milestone Jul 4, 2019
@evdokimovs evdokimovs self-assigned this Jul 4, 2019
@evdokimovs
Copy link
Contributor Author

@alexlapa , @tyranron ,

С #[automatically_derived] clippy все равно ругается, поэтому оставлю вариант с #[allow()].

@evdokimovs
Copy link
Contributor Author

@alexlapa ,

На данный момент у нас в proto спеке во всех эндпоинтах есть поле dst. Насколько оно нам вообще нужно? На данный момент же получатель находится сам на стороне сервера. Это пережитки прошлого и стоит удалить эти поля, или есть какой-то другой смысл, которого я не знаю?

@tyranron
Copy link
Member

@evdokimovs the referred spec is just an example. The dst field is used in some media elements to show how it could be declared. The meaning of the src and dst fields maps directly to source and sink notions of GStreamer elements (ability to specify elements where from/to a media stream should flow).

If you do not need these fields in the current implementation at all, then just do not declare them.

@evdokimovs
Copy link
Contributor Author

@tyranron ,

@evdokimovs the referred spec is just an example. The dst field is used in some media elements to show how it could be declared. The meaning of the src and dst fields maps directly to source and sink notions of GStreamer elements (ability to specify elements where from/to a media stream should flow).

If you do not need these fields in the current implementation at all, then just do not declare them.

Хорошо, спасибо.

@evdokimovs
Copy link
Contributor Author

evdokimovs commented Jul 17, 2019

FCM

Implement dynamic Control API exposed via gRPC (#33, #32)

- add gRPC Control API server
- implement 'Create', 'Delete', 'Get' methods of Control API
- add client for manual tests of gRPC control API
- add E2E tests for gRPC Control API
- add '[server.control.grpc]' section to configure Control API gRPC server
- add 'server.client.http.public_url' option to configure public URL of Client API HTTP server

Additionally:
- rename '[server]' section of Client API HTTP server as '[server.client.http]'
- add global app context
- use tinyurl.com instead of tiny.cc in docs
- add printing info about module and line in logs
- fix medea-demo:edge Dockerfile

@evdokimovs evdokimovs marked this pull request as ready for review July 17, 2019 15:04
@evdokimovs evdokimovs requested a review from alexlapa July 17, 2019 15:04
@alexlapa alexlapa added the waiting: materials There is a need for additional actions/info outside the issue/PR to continue its solving. label Jul 19, 2019
Copy link
Collaborator

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

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

@evdokimovs ,

Необходимо актуализировать ветку. И вешаю waiting до завершения 0.1.0 milestone.

@evdokimovs
Copy link
Contributor Author

@alexlapa ,

Если сделать запрос ровно в момент выключения gRPC сервера, то вернется такая ошибка:

RpcFailure(RpcStatus { status: Unavailable, details: Some("Socket closed") }).

@tyranron tyranron force-pushed the master branch 3 times, most recently from 8f9956c to 7432f56 Compare August 21, 2019 14:55
@evdokimovs evdokimovs requested a review from alexlapa October 16, 2019 14:28
Copy link
Collaborator

@alexlapa alexlapa left a comment

Choose a reason for hiding this comment

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

@evdokimovs ,

Discussed:

  1. Sync with mainline
  2. Some minor changes (added todos)

@alexlapa
Copy link
Collaborator

@evdokimovs ,

Там косячек с medea-demo:edge, засуньте туда в Dockerfile FROM alexlapa/rust-beta:1.39-beta.5, пожалуйста.

@evdokimovs evdokimovs requested a review from alexlapa October 18, 2019 15:08
@alexlapa alexlapa requested a review from tyranron October 21, 2019 09:02
Copy link
Member

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

FCM

Implement dynamic Control API exposed via gRPC (#33, #32)

- integrate gRPC Control API server
- imp 'Create', 'Delete', 'Get' methods of Control API
- add temporary client for manual tests of gRPC Control API
- create E2E tests for gRPC Control API
- add '[server.control.grpc]' section to configure Control API gRPC server
- rename '[server]' section of Client API HTTP server as '[server.client.http]'
- add 'server.client.http.public_url' option to configure public URL of Client API HTTP server

Additionally:
- expose configuration changes in 'medea-demo' Helm chart values and bump up its version to 0.2.0
- use tinyurl.com instead of tiny.cc in docs
- remove installing 'rustfmt' on CI as it now goes in default profile for rustup
- fix missing CMake installation in Dockerfile
- describe explicit env var names in configuration file
- provide global AppContext
- add module path and file line number to logs
- refactor types and attributes usage in 'conf' module and split tests by submodules

bind_port: 8080
client:
http:
public_url: ws://127.0.0.1:8080/ws
Copy link
Member

Choose a reason for hiding this comment

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

Changes to Helm chart, especially breaking ones, should go with chart's version bump.

@@ -36,10 +36,10 @@ message CreateRequest {
}
}

// Request with many Elements IDs.
// Request with many Elements Full IDs (FIDs) elements.
Copy link
Member

Choose a reason for hiding this comment

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

Please, be more careful with docs formulation.

message CreateRequest {
// ID of the Element to be created..
string id = 1;
// Full ID (FID) of the Element in which will be created provided element.
Copy link
Member

Choose a reason for hiding this comment

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

When dealing with abbreviations, it's better to do vice versa: first goes abbreviation and then full words in parentheses.

FID (full ID)

Such approach leads people towards abbreviation use.

@@ -99,7 +99,7 @@ impl Actor for WsSession {
/// Starts [`Heartbeat`] mechanism and sends [`RpcConnectionEstablished`]
/// signal to the [`Room`].
fn started(&mut self, ctx: &mut Self::Context) {
debug!("Started WsSession for member {}", self.member_id);
debug!("Started WsSession for Member [id = {}]", self.member_id);
Copy link
Member

Choose a reason for hiding this comment

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

You do not need interpolation/formatting in logs when you're using structured logs already. Leverage the power of abstractions you're using.

Copy link
Member

Choose a reason for hiding this comment

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

Consider to refactor it in a separate PR. All lines like the one above should be refactored in the following manner:

debug!("Started WsSession for Member"; "Member.id" => self.member_id);

This will serve good both for reading in console, indexing in logs aggregation databases and reducing ops per log line.

.travis.yml Outdated
@@ -20,8 +20,6 @@ stages:

jobs:
allow_failures:
- rust: nightly
stage: check
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, rustfmt now goes in a default profile for rustup, however, it still can be missing in nightly builds.


/// Some error happened in [`Member`].
#[display(fmt = "{}", _0)]
MemberError(MemberError),
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary. Omitting attribute will result in the same trvivial implementation.

///
/// Returns [`AuthorizationError::InvalidCredentials`] if [`Member`]
/// Returns [`Err(AuthorizationError::InvalidCredentials)`] if [`Member`]
Copy link
Member

Choose a reason for hiding this comment

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

I doubt that will render in docs OK.

@evdokimovs evdokimovs changed the title WIP: Dynamic control API exposed via gRPC (#32) Dynamic control API exposed via gRPC (#32) Oct 25, 2019
@evdokimovs evdokimovs merged commit e71c31c into master Oct 25, 2019
@evdokimovs evdokimovs deleted the 32-grpc-dynamic-control-api branch October 25, 2019 13:45
@tyranron tyranron added enhancement Improvement of existing features or bugfix k::config Related to application configuration labels Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement of existing features or bugfix feature New feature or request k::api Related to API (application interface) k::config Related to application configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic control API exposed via gRPC
3 participants