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

Simplify gateway backend registration #5111

Merged
merged 1 commit into from Oct 27, 2017
Merged

Conversation

@timonwong
Copy link
Contributor

@timonwong timonwong commented Oct 24, 2017

Description

Motivation and Context

When I'm trying to add a new gateway backend in #5103, several files need to be touched, which IMHO is not very convenient.
In this pull request, gateway backend implementations only need to focus their own scope, and
just invoke mustRegisterGatewayBackend() function in their init().

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch from 4dd4004 to a187c4c Oct 24, 2017
@codecov
Copy link

@codecov codecov bot commented Oct 24, 2017

Codecov Report

Merging #5111 into master will increase coverage by 0.56%.
The diff coverage is 56.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5111      +/-   ##
==========================================
+ Coverage   61.25%   61.82%   +0.56%     
==========================================
  Files         193      153      -40     
  Lines       28411    26216    -2195     
==========================================
- Hits        17404    16208    -1196     
+ Misses       9742     8808     -934     
+ Partials     1265     1200      -65
Impacted Files Coverage Δ
cmd/globals.go 100% <ø> (ø) ⬆️
cmd/web-handlers.go 74.96% <33.33%> (+0.1%) ⬆️
cmd/gateway-main.go 19.76% <7.2%> (+7.19%) ⬆️
cmd/gateway-gcs.go 12.04% <74.6%> (+5.37%) ⬆️
cmd/gateway-b2.go 7.67% <84.78%> (+7.67%) ⬆️
cmd/gateway-s3.go 16.89% <84.9%> (+9.52%) ⬆️
cmd/gateway-azure.go 24.2% <85.18%> (+5.72%) ⬆️
cmd/storage-rpc-client.go 36.63% <0%> (-41.09%) ⬇️
cmd/update-notifier.go 58.33% <0%> (-28.34%) ⬇️
cmd/storage-rpc-server.go 52.63% <0%> (-26.32%) ⬇️
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 758d545...a187c4c. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Oct 24, 2017

Codecov Report

Merging #5111 into master will increase coverage by 0.46%.
The diff coverage is 54.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5111      +/-   ##
==========================================
+ Coverage   61.36%   61.82%   +0.46%     
==========================================
  Files         194      153      -41     
  Lines       28567    26237    -2330     
==========================================
- Hits        17529    16220    -1309     
+ Misses       9768     8821     -947     
+ Partials     1270     1196      -74
Impacted Files Coverage Δ
cmd/globals.go 100% <ø> (ø) ⬆️
cmd/gateway-startup-msg.go 90% <100%> (ø) ⬆️
cmd/web-handlers.go 74.96% <33.33%> (+0.1%) ⬆️
cmd/gateway-main.go 22.07% <45%> (+9.45%) ⬆️
cmd/gateway-gcs.go 11.44% <46.15%> (+4.8%) ⬆️
cmd/gateway-s3.go 16% <49.39%> (+8.64%) ⬆️
cmd/gateway-azure.go 23.57% <57.53%> (+5.12%) ⬆️
cmd/gateway-b2.go 6.77% <85%> (+6.77%) ⬆️
cmd/storage-rpc-client.go 36.63% <0%> (-41.09%) ⬇️
cmd/update-notifier.go 58.33% <0%> (-28.34%) ⬇️
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03df692...90f0c8a. Read the comment docs.

@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch 3 times, most recently from 9272afa to 47b8a77 Oct 24, 2017
@nitisht nitisht requested review from harshavardhana and balamurugana Oct 24, 2017
Copy link
Member

@harshavardhana harshavardhana left a comment

Overall a nice change, but some comments about style and structure.

}

return nil, fmt.Errorf("Unrecognized backend type %s", backendType)
return nil, fmt.Errorf("unrecognized backend type %s", backendType)

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

Capitalize the strings for errors, this is to be consistent with our code base.

`

mustRegisterGatewayBackend(gatewayBackendEntry{
Name: "s3",

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

we can use s3Backend const here..

mustRegisterGatewayBackend(gatewayBackendEntry{
Name: "s3",
Cmd: cli.Command{
Name: "s3",

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

same as above and can be replicated for all gateway backends..

@@ -102,12 +161,13 @@ type s3Objects struct {
}

// newS3Gateway returns s3 gatewaylayer
func newS3Gateway(host string) (GatewayLayer, error) {
func newS3Gateway(args cli.Args) (GatewayLayer, error) {

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

if a gateway takes only one argument then we don't need to expand the scope here by adding more args. This refactor may have led to that. But taking in input param as host is more meaningful here.

Name gatewayBackend
Cmd cli.Command
New func(args cli.Args) (GatewayLayer, error)
IsExperimental bool

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

We don't need to call this IsExperimental just say it as Experimental is fine.

}

gatewayBackendReg = map[gatewayBackend]gatewayBackendEntry{}

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

We can avoid map and make this more safer by using a slice instead?

This comment has been minimized.

@timonwong

timonwong Oct 24, 2017
Author Contributor

We can avoid map and make this more safer by using a slice instead?

Not sure, would you mind telling why a slice is more safer in this scenario?

)
// Represents the type of each item in gateway backend registry.
type gatewayBackendEntry struct {
Name gatewayBackend

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

there is no reason for this type if there are no constants anymore.

return errors.New("field 'New' cannot be nil")
}

if e.Cmd.Action != nil {

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

This check is sort of incorrect with the overall usage and pattern, There is no reason we should add this check as it might be frivolous indeed. We should run a Validate() func right after the Action is registered, or just remove verifying Action.

return fmt.Errorf("duplicate backend type found in registry: %s", string(backendType))
}

entry.Cmd.Action = gatewayMainAction(backendType)

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

This additional code here is not needed, modifying input structs causing side-affects is not a good idea usually. We should simply move this code to mustRegisterGatewayBackend() - gatewayMainAction("s3") for example.

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

So all you would have to do would to do is just simply do validation here and avoid mutation.

@@ -292,12 +354,13 @@ func azureParseBlockID(blockID string) (partID, subPartNumber int, uploadID, md5
}

// Inits azure blob storage client and returns AzureObjects.
func newAzureLayer(host string) (GatewayLayer, error) {
func newAzureLayer(args cli.Args) (GatewayLayer, error) {

This comment has been minimized.

@harshavardhana

harshavardhana Oct 24, 2017
Member

The issue about changing input params similar to my comment on s3 gateway

@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch 2 times, most recently from 913aabe to 5c75965 Oct 24, 2017
Copy link
Member

@balamurugana balamurugana left a comment

We could have an interface approach to solve the problem. That way each gateway backend is completely independent. Below is the approach

gateway-main.go:

  • Add an interface
type Gateway interface{
    Name() string
    NewGatewayLayer() (GatewayLayer, error)
}
  • Rename gatewayMain() to startGateway()

gateway-s3.go:

  • In init() add subcommand by gatewayCmd.Subcommands = append(gatewayCmd.Subcommands, cli.Command{...})
  • Have S3Gateway compatible to Gateway interface
type S3Gateway struct {
    host string
}

func (g *S3Gateway) Name() string {
    return "s3"
}

func (g *S3Gateway) NewObjectLayer() (GatewayLayer, error) {
    // log.Println(colorYellow("\n               *** Warning: Not Ready for Production ***"))
    // have newS3Gateway() here
}

This logic needs to be done in all gateway code

@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Oct 25, 2017

@balamurugana

I think we should not access gatewayCmd.Subcommands directly in each gateway implementation.
My ultimate goal (perhaps) is the possibility of adding "plugin" support, so we can easily opt-in/opt-out.

What about this?

type GatewayCommand interface {
	// Name is the global unique name for each gateway.
	Name() string
	// A short description of the usage of this command.
	Description() string
	// HelpTemplate returns the text template for the command help topic.
	HelpTemplate() string
	// Parse parse the command line context.
	Parse(ctx *cli.Context) error
	// NewGatewayLayer returns a new gateway layer.
	NewGatewayLayer() (GatewayLayer, error)
}

func RegisterGatewayCommand(command GatewayCommand) error {
	// We should not have multiple subcommands with same name.
	for _, c := range gatewayCmd.Subcommands {
		if c.Name == command.Name() {
			return fmt.Errorf("duplicate gateway command: %s", command.Name())
		}
	}

	gatewayCmd.Subcommands = append(gatewayCmd.Subcommands, cli.Command{
		Name:               command.Name(),
		Usage:              command.Description(),
		Action:             gatewayCommandMain(command),
		CustomHelpTemplate: command.HelpTemplate(),
		Flags:              append(serverFlags, globalFlags...),
		HideHelpCommand:    true,
	})
	return nil
}
@balamurugana
Copy link
Member

@balamurugana balamurugana commented Oct 25, 2017

@timonwong This approach brings a cyclic usage i.e. gateway-main.go calls a method on registration. To make things simple, you can have a registerCommand() in gateway-main.go and every gateway layer calls this function for its registration for cli package. This way each knowledge is abstracted within each gateway layer.

@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch 4 times, most recently from f9e0147 to 5110625 Oct 25, 2017
@deekoder deekoder force-pushed the minio:master branch from 1f02c70 to db3fed2 Oct 25, 2017
@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch 3 times, most recently from 42632c9 to 6becd79 Oct 26, 2017
@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Oct 27, 2017

Can you have a look latest commit, though I'm afraid it's not very feasible, because:

  1. RegisterGatewayCommand() requires a GatewayCommandDesc with command line usage, help, and a function to parse command line and create a Gateway. (Because I don't want to call gatewayMain in each gateway implementation)
  2. gateway-main.go handles actual command action, then call Gateway:NewObjectLayer() to fully bring up a gateway layer.
// Add more backends here.
)
// GatewayCommandDesc is descriptor for gateway command.
type GatewayCommandDesc struct {

This comment has been minimized.

@balamurugana

balamurugana Oct 27, 2017
Member

Other than cli package, this information is nowhere required. You could directly pass cli.Command{} from each gateway to MustRegisterGatewayCommand() and RegisterGatewayCommand().

@balamurugana
Copy link
Member

@balamurugana balamurugana commented Oct 27, 2017

Can you have a look latest commit, though I'm afraid it's not very feasible, because:

RegisterGatewayCommand() requires a GatewayCommandDesc with command line usage, help, and a function to parse command line and create a Gateway. (Because I don't want to call gatewayMain in each gateway implementation)
  1. GatewayCommandDesc is not required. See my above comment.
  2. gatewayMain() is a common code to know how to start a gateway. Calling startGateway() in each gateway is correct
gateway-main.go handles actual command action, then call Gateway:NewObjectLayer() to fully bring up a gateway layer.

Note that all gateway-main and gateway-*.go are same package. Its not required to visualize as separate layer.

@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch from 6becd79 to a147717 Oct 27, 2017
@timonwong timonwong force-pushed the timonwong:cleanup-gateway branch 3 times, most recently from 4970069 to 90f0c8a Oct 27, 2017
@timonwong
Copy link
Contributor Author

@timonwong timonwong commented Oct 27, 2017

@balamurugana Done, just check the latest commit

Copy link
Member

@balamurugana balamurugana left a comment

LGTM

@deekoder deekoder merged commit 6400f50 into minio:master Oct 27, 2017
5 checks passed
5 checks passed
@codecov
codecov/patch 73.59% of diff hit (target 61.36%)
Details
@codecov
codecov/project 61.6% (+0.24%) compared to 03df692
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
timonwong added a commit to timonwong/minio that referenced this pull request Oct 28, 2017
In minio#5111, 'help' subcommand was moved to `startGateway` during review,
but not get restored then. So the "help" subcommand won't function if
credentials are not provided.
dvstate added a commit to dvstate/minio that referenced this pull request Nov 2, 2017
@timonwong timonwong deleted the timonwong:cleanup-gateway branch Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants