[FAB-17960] Ch.Part.API: Implement filerepo #1441
Conversation
|
Pushed an initial update addressing the comments from above. Will update once #1457 is merged to use the fileutil functions for some of the file system operations. |
8282694
to
a8f1b33
Compare
orderer/common/filerepo/filerepo.go
Outdated
| type repoOption func(r *Repo) | ||
|
|
||
| func WithTransientFileMarker(marker string) repoOption { | ||
| return func(r *Repo) { | ||
| r.transientFileMarker = marker | ||
| } | ||
| } |
There was a problem hiding this comment.
Function pointers are not good for readability IMO. It is good when we want to add hooks (heavily used in linux kernel). I yet to read the updated code again since I reviewed a few days back. Is there no way to remove this?
There was a problem hiding this comment.
I could change the New function to just take an optional variatic transientFileMarker list instead and read the first element in that list if it exists to override the default transientFileMarker. I was mainly trying to avoid just accepting a transientFileMarker argument and ignoring it if it was an empty string since the default would probably be to not pass an override.
There was a problem hiding this comment.
This is using the OptionFunc pattern for extensible construction. I'm not sure why that's considered difficult to read. It's a flexible, pragmatic, and relatively common way of handling object construction in a friendly, backwards compatible way.
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
The same pattern can be used in place of the "builder" pattern to minimize error handling to one call while building up a bunch of changes.
There was a problem hiding this comment.
Whenever there is a function pointer and it's usage, I always had to spend some extra time to understand the code. If there is a simple way like NewServer(config) or NewServer(config1, confg2), I spend less time to read and understand. This is what I meant regarding the readability. If this is a common pattern and used widely, then I might probably improve my readability skill once I find and read many such patterns.
As I mentioned in my first set of review, I do not see a strong reason to configure the transientFileMarker. It is just an internal logic to the filerepo. Given that, I am not convinced that we need a function pointer or configurable option.
a21e3d4
to
80c6cfa
Compare
|
Updated to use |
orderer/common/filerepo/filerepo.go
Outdated
| } | ||
|
|
||
| for _, opt := range opts { | ||
| if err := opt(repo); err != nil { |
There was a problem hiding this comment.
additional response to @sykesm question on why I find this hard to read.
- I did go to definition by keeping the cursor on
opt. Nothing happened. - I needed to track to opts and reach
ReadOption - Then, I find that ReadOption is a function pointer and it provides a list of functions which the consumer could have called to get a function implementation that sets a value for a variable.
- The returned function implementation is called here.
I understand that this allow the caller to configure only the required variables (i.e., optional variables) but adds more time to the code readability (& multi-level functions). As I do not see any strong reason for the caller to set transientFileMarker (as it is internal to the filerepo pkg), I am not convinced that this added code complexity is required. The default ~ looks fine to me. If others have a strong opinion that the added code complexity is justified and transientFileMarker needs to be configured by the caller, I will step aside.
There was a problem hiding this comment.
I'll remove the configurability for now as I don't see an immediate need to override the default value. We can always revisit this down the line if an override is needed.
There was a problem hiding this comment.
In this instance, fine. In general, this is the wrong way.
When it comes to constructing objects, we have:
- Zero values
- Struct literals that require nothing more than wiring
- Constructors that take no arguments
- Constructors that take specific arguments
- Constructors that can evolve
Option 1 is best. It's the "make the zero value useful" goal of of the standard library.
Option 2 is good when evolving from option 1. This allows us to create a default behavior with the zero value and add customizability after the fact by injecting something to override behavior. When we set a field, we have an explicit name at the call site so we know what we're providing. We see a lot of this in the http package.
Option 3 is okay but limiting. It means that we're constructing something and there's no way to control it. That implies we end up having to modify the state after construction to make things useful or build global state around it to influence it.
Option 4 is rigid and confusing. The rigidity comes from the fact that there's never a backward compatible way to add or remove constructor arguments without impacting callers. It's confusing when we get long lists of arguments that share the same type. In this particular case, it looks like we have n strings. So, at the call site, I need to figure out the order and what they mean.
Option 5 is good. In this model we can easy extend the functionality of constructors. Each option is named so when we make the call, we know what we're providing. The options allow better encapsulation of the object as even the names of the fields we use to hold dependencies is not exposed to the caller.
Given how prevalent the option func pattern is in libraries, we should all be able to recognize it for what it is and embrace the benefits it provides when it comes to evolving a code base.
There was a problem hiding this comment.
Thanks, @caod123 for removing this configuration option.
@sykesm Thanks for the nice summary. Especially what you mentioned in option 4 and 5 are nice -- no need to break backward compatibility when we add new configuration option -- didn't strike me. While optionalal func pattern might add a bit of overhead while reading it for the first time (as in my case), we will adjust to it as we see more patterns. I hope you don't mind that I am copying your suggestions to the google doc that I maintain so that these suggestions/recommendations are not lost within the PR comments.
3345dbd
to
2dd4d05
Compare
|
@cendhu could you do another pass of this and let me know if it looks good to you? There hasn't been much activity on this since last week. |
| return nil, err | ||
| } | ||
|
|
||
| fileRepoDir := filepath.Join(repoParentDir, fileSuffix) |
There was a problem hiding this comment.
Nit. Using fileSuffix both as a sub-directory name and also as a suffix to the file name within the sub directory kind of confuses me while reading the code. I think the fileSuffix denotes the type of content being stored. One such type is the join block. We then create a sub-folder for the type and also add the type as a suffix to each file stored in the sub-folder.
Anyway, I do not have an alternative to the current folder structure that is shown below. Maybe the joinblock suffix looks redundant.
/repo/joinblock
/repo/joinblock/channel1.joinblock
/repo/joinblock/channel2.joinblock
/repo/joinblock/channel3.joinblock~
There was a problem hiding this comment.
Having separate subfolders helps to isolate different file repos from operating on the same folder. The suffix is still useful for filtering in case other files end up in the folder through other means.
There was a problem hiding this comment.
Having separate subfolders helps to isolate different file repos from operating on the same folder. The suffix is still useful for filtering in case other files end up in the folder through other means.
What other means? You mean admin creates some files manually?
There was a problem hiding this comment.
Sure, I don't see it really happening but in the off chance the admin may backup a joinblock to the same directory or for some other reason accidentally move a separate file to the folder. In the end it doesn't really hurt to have the suffix in addition to the subfolder even if it introduces some minor redundancy. As for the subfolder existence itself, Yoav pointed out in a previous comment that having separate subfolders would speed up the scan and avoid filesystem sync issues if we use two file-repo's in the same process with different suffixes
* Introduce a general purpose file repo for persisting join block data as well as supporting CFT on join and remove operations from the registrar and file ledger. Signed-off-by: Danny Cao <[email protected]>
There was a problem hiding this comment.
Thanks, Danny. I am still not sure about the following two points
- having a fileSuffix as both folder name and the file suffix
- having a filter logic in the List()
If required, it can be fixed in a separate PR after the discussion and need not to block this PR.
In future, please try to not rebase in the middle of reviews. Otherwise, it is quite hard to find out what changed compared to the initial commits.
Type of change
Description
Introduce a general purpose file repo for persisting join block data
as well as supporting CFT on join and remove operations from the
registrar and file ledger.
Related issues
Task: FAB-17960
Story: FAB-15711
Epic: FAB-17712