Align on some rust conventions

[slinkydeveloper]

• Structured error names should be consistent. Should we call error names using [module]Error or simply Error and use qualified imports such as [module]::Error?
• Service names should be consistent. Should we call services using [module]Service or simply Service and use qualified imports such as [module]::Service? This one should probably be aligned with the error names decision.
• Unify module structure in code base #220
• Right now we're using common or other crates to separate api and implementation of components. This is something we're doing for meta and worker (and invoker). I think it makes sense to align on that on the same fashion of the storage, where we have api and impl crates. It probably makes sense to have them as subdirectories, e.g.

- storage
  - api
  - impl
- meta
  - api
  - impl
  - [.. maybe other modules ..]
- worker
  - api
  - impl
  - [.. maybe other modules ..]

• Util crate names: [..]_util vs [..]_utils
• Reconsider [WIP] Macro to define interfaces abstracting channels #166
• Reconsider Use rustfmt nightly and enable group_imports #242

Tasks

Beta Give feedback

1. Document agreed upon conventions for codebase #542
   +0
   
2. Split invoker in api/impl modules #546
   enhancement invoker +2
   

[slinkydeveloper]

WDYT about the aforementioned points @tillrohrmann ? In particular about error names, service names and the [component]_api/[component_impl] splitting.

[tillrohrmann]

I think it is a good idea to improve our codebase's consistency.

1. I don't have a very strong opinion here. On the one hand I do like not repeating the module name for the struct when working on it. On the other hand, looking a specific service up will then always require module::Error otherwise one has a hard time finding the right Error. Maybe the latter point is simply a matter of practice. When re-exporting the Error would re-export it as ModuleError, right?
2. Same here.
3. Done
4. Splitting the interfaces from the implementation sounds reasonable to me. The one thing that is not clear to me is what interfaces let's say worker/api would contain? Are those the interfaces that the worker/impl uses to do its job (e.g. a Storage interface) or are those the interfaces that are being implemented by the worker/impl? From an architectural point of view the business logic should define interfaces that the infrastructure/peripherial components implement (which would be the case that worker exposes interfaces for its I/O, for example).
5. No strong preference.
6. Unsure whether this won't increase the entrance hurdle into our code base. Code should be optimized for readability and not writability.
7. Relying on unstable features that will most likely not make it into stable sounds not like a good idea to me.

[slinkydeveloper]

When re-exporting the Error would re-export it as ModuleError, right?

Yes, I guess my question is: do we rename when consuming, or when exporting? I personally like renaming when cosuming: restate_meta exports its service struct as restate_meta::Service, but inside the worker you rename it use restate_meta::Service as MetaService.

[slinkydeveloper]

The one thing that is not clear to me is what interfaces let's say worker/api would contain? Are those the interfaces that the worker/impl uses to do its job (e.g. a Storage interface) or are those the interfaces that are being implemented by the worker/impl? From an architectural point of view the business logic should define interfaces that the infrastructure/peripherial components implement (which would be the case that worker exposes interfaces for its I/O, for example).

Haven't put much thought into this TBH. There are cases where I can clearly see when a type needs to be inside the _api module: input/output messages, Handle/Sender interfaces/structs. I guess we can go on a case by case basis on this one, unless you have a better option.

[tillrohrmann]

Yes, I guess my question is: do we rename when consuming, or when exporting? I personally like renaming when cosuming: restate_meta exports its service struct as restate_meta::Service, but inside the worker you rename it use restate_meta::Service as MetaService.

The one thing that I don't like about renaming is that it makes things harder to back track because it introduces another layer of indirection. With this pattern, people will have to look at the use statement to find out the actual name if they want to go to the definition of the struct.

[slinkydeveloper]

So what do you think we should do then?

• no renaming at all
• rename on exporting
• rename on consumption

Right now I merged #528 following the renaming on consumption :) But can change, no strong opinion here, I do like renaming at least on exporting, as it makes less verbose the names within the crate, and it makes easier (at least for me) to reason about the crate apis.

[igalshilman]

Right now we're using common or other crates to separate api and implementation of components. This is something we're doing for meta and worker (and invoker). I think it makes sense to align on that on the same fashion of the storage, where we have api and impl crates. It probably makes sense to have them as subdirectories, e.g.

I like this approach, it is very common practice to separate api and service/core etc' 👍

I think that they rather to be separate crates, as they minimize the dependency scope

[slinkydeveloper]

For the renaming topic, I've checked a couple of projects:

• tikv
• linkerd
• stdlib
• hyper

All these rename on consumption the Error type of the respective modules. So I guess it makes sense to rename on consumption for us as well?

[slinkydeveloper]

This one is done, we can close it