rsm: proposal for refactoring

[coufalja]

• Removed NALookup
  • NALookup provide questionable perf benefits while adding complexity
  • Perf benefit could be achieved by simply passing []byte to the regular Lookup method
• Removed IHash
  • Its use was only internal to a single unit test and was not exposed through public API of a module
• Removal of Stale read warning
  • This should be a responsibility of a caller not a library, it is difficult to get rid of the warning in the logs if one does not want to set the whole logger to error just to get rid of it.
• Fixed a couple of CVEs by bumping libs

[lni]

@coufalja thanks for the PR, I will have to look into every item in the coming weekend, but in general I agree with what you are trying to do and expect most of them (if not all of them) to be accepted.

From a user's point of view, do you have anything else to suggest? Like which part of the API you hate the most? Do you feel the docs to be adequate? Will really appreciate all such feedbacks.

[coufalja]

Thanks for asking, first I would like to say that I like the library a lot and that it provides a great value to its users.

But after using dragonboat extensively a couple of things bothered me:

• Unexported/Internal method that I had to mimic
  • nodehost.go#getRequestState - It would be great if either exported or if RequestState provided a Wait method. It is easy to miss some cases so I would rather see a library provided default way to wait for result to be available.
  • internal/rsm/encoded.go#GetPayload - Give that there is an API access to pb.Entry it would be great to not have to mimic the Cmd parsing in case that one wants to obtain the value.
• NodeHost configuration
  • It is difficult to know which fields should be filled when and which are in effect when using different parts of the library (e.g. when Using Tan vs Pebble)
  • The configuration is propagated through the whole package stack and makes writing plugins a bit difficult
  • If I may suggest it would make maybe a better design if every plugin (Transport/LogDB/Registry) had its own config instead of plugins being factories accepting a common config. It is more work for library user but IMO more straightforward and clear.
• NewNodeHost starting go routines
  • My take is that a library should not start coroutines in seemingly constructor-like method (and rather should provide Start/Serve or alike methods)
• internal/settings package
  • Library should not rely on hardcoded files and their location in a current dir for settings override. All settings should be provided by passing code, if library user wants to use a file so be it but on user terms.
  • Initialization is done on a package init phase, very difficult to test different settings.
• closely tied with Pebble dependency
  • the dependency sometimes get in a way especially the VFS package
  • even when the pebble is used in unrelated way to dragonboat in the same project
  • as pebble changes its API it is necessary to update dragonboat codebase as well to take advantage of newer Pebble features

I am happy to provide PRs if we agree on tackling any of these items as well as I am happy to provide support in getting the V4 out of the door.

[codecov-commenter]

Codecov Report

Patch coverage has no change and project coverage change: +0.04 🎉

Comparison is base (023bafb) 84.53% compared to head (d5b931a) 84.56%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   84.53%   84.56%   +0.04%     
==========================================
  Files          10       10              
  Lines        4264     4254      -10     
==========================================
- Hits         3604     3597       -7     
+ Misses        404      402       -2     
+ Partials      256      255       -1     

Impacted Files	Coverage Δ
nodehost.go	86.95% <ø> (+0.07%)	⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[lni]

Thanks @coufalja for the detailed input, really appreciated. Let's start the discussion, hopefully we can have a plan in a couple of weeks.

nodehost.go#getRequestState, It would be great if either exported or if RequestState provided a Wait method.

Users are expected to wait for different events simultaneously from different libraries for different purposes, e.g. a user might wait for the result of the proposal and a possible incoming Unix signal in the same select statement. A simple wait method is not going to help much as it is basically forcing them to just wait for dragonboat event. If that is the case, why not just use the synchronized variant of the API SyncPropose?

internal/rsm/encoded.go#GetPayload - Give that there is an API access to pb.Entry it would be great to not have to mimic the Cmd parsing in case that one wants to obtain the value.

Dragonboat's internal data structure & encoding method should be completely separated from users'. It is up to users to decide how they want to encode their payload, dragonboat doesn't care.

Note that both the above two items are about giving users the max flexibility, the goal is not to let users write slightly less code by putting restrictions. For a particular user, he/she might feel certain internal methods can be useful/reused, but that is probably not the case for other users. Exposing too many such internal stuff produces a heavy API - the exact problem I am trying to solve in v4.

It is difficult to know which fields should be filled when and which are in effect when using different parts of the library (e.g. when Using Tan vs Pebble)

You are right.

Other than better documentation & examples, might be able to make that more explicit by adding fields like TanConfig which point to a configuration object just for Tan.

The configuration is propagated through the whole package stack and makes writing plugins a bit difficult

Sorry but I don't quite get this one. It is propagated through the whole package so no matter what component you want to work on, you always get the full configuration. Can I just ask what make this more difficult to work with?

My take is that a library should not start coroutines in seemingly constructor-like method (and rather should provide Start/Serve or alike methods)

I am not convinced on this one. If goroutine is considered as a type of resources, a popular approach is to obtain it during initialization and the constructor is a typical place for doing it. In fact, RAII explicitly requires such resources to be obtained in constructor.

I feel that the real problem for dragonboat is that it is not consistent. Some structs might have all such resources obtains in its constructor like method, while others do it in their Start() method. I am also a little uncomfortable with the fact that there are both Stop() and Close(), it is confusing at best. Any suggestions on this?

Library should not rely on hardcoded files and their location in a current dir for settings override. All settings should be provided by passing code, if library user wants to use a file so be it but on user terms.

I won't worry too much on this one, because the underlying message is "don't change those settings unless you really need to".

There are some hooks that allows you to change those values during initialization. It is used by dragonboat's testing framework drummer

closely tied with Pebble dependency

Yes, all your concerns here are valid but the real problem here is that Pebble doesn't follow Go's versioning convention. This means there is no any guarantee on Pebble's API stability, it can change at any time without any warning. For packages that do follow Go's versioning convention, it wouldn't be a problem as you can have multiple versions (e.g. v2 & v3) used by different part of your application and you have the peace in mind that v2 changes won't break your application as such breaking changes would change its major version number.

Please also understand that there was no alternative choice at the time - not any native Go embedded db has the same level of quality as Pebble. That is why Pebble was picked.

Tan is definitely the future as it doesn't have all those key-value store related overheads, it can be made much easier to configure, its performance is much easier to predict. To me, I really want to just remove the Pebble support altogether, sadly I can't do it because of backwards compatibility reasons.

I personally suffered from the above described Pebble versioning issue multiple times, but I don't have a solution here. Any suggestion would be really appreciated.