example/helloworld: rename proto directory

[chalin]

Use the name proto for the folder containing .proto files, as is done in other examples.

(There seems to be too much stuttering in paths like examples/helloworld/helloworld/helloworld.pb.go)

[dfawley]

PTAL at the travis failures (I think the mock code needs to be regenerated to pick up the new package name).

[menghanl]

Not sure how serious this is, but this is an API change. If users import these proto packages (for what??? their helloworld???), they will be broken.

[menghanl]

Another reason to not make this change: go's directory is usually the same name as the package. helloworld.pb.go is in package helloworld. Keeping the folder named helloworld is go idiomatic.

[dfawley]

Not sure how serious this is, but this is an API change. If users import these proto packages (for what??? their helloworld???), they will be broken.

That's true, moving the pb.go files is a breaking change. I'm not too worried about this since it's an example and I don't see any external repos with references in sourcegraph. We do seem to have about 10-15 references in our own code, though.

Regarding package name matching directory name, though, this is a good point. Code gets weird when the two don't match:

package foo // import as: "some/package"

package main

import "some/package"

...
   foo.Bar()
...

This "works" but it's impossible to determine where "foo" came from.

[chalin]

Another reason to not make this change: go's directory is usually the same name as the package.

Fixed.
And reworked and regenerated the mocks.

Now, in the latest Travis build for this PR, #9808, all jobs are passing except TESTEXTRAS, which I'll look into next week:

[dfawley]

I'm not sure I agree this is a good change. The breakage is not the problem - naming the package "proto" is unusual, or having it not match the directory name is also suboptimal. It may be best to leave it as-is.

[chalin]

grpc-go/profiling uses naming conventions like what is being proposed here:

The package and directory names match (they are both proto) for profiling/proto/service.proto:

// profiling/proto/service.proto excerpt:
option go_package = "google.golang.org/grpc/profiling/proto";

The profiling/cmd/main.go file imports it like this:

package main

import (
	"os"

	"google.golang.org/grpc/grpclog"
	ppb "google.golang.org/grpc/profiling/proto"
)

Thoughts?

[dfawley]

I'm fine with that approach, though I also don't see much value in changing it, and there are risks. @menghanl?

Note that the proto package for profiling is not named "proto"; just the Go package. Also, we can't rename the helloworld proto package, because it is used in the other languages as well, and there are interop cases where they need to be able to interact.

[chalin]

Ok, we can put this on ice for a bit -- I'll address some of your questions & comments later. #3546 is more important IMHO.

[menghanl]

I'm also not a big fan of renaming the proto package.
If we are trying to setup a best practice example, I would think package helloworld is better than package proto (I'm talking about the proto package, not the go package).

Adding option go_package = "google.golang.org/grpc/profiling/proto"; to rename the go package would be fine. But keeping the proto package consistent with go package is more desirable.

A third option: move the proto folder into a proto folder. So the path becomes examples/helloworld/proto/helloworld/helloworld.proto.

[dfawley]

Closing for now since we don't have an agreed-upon approach for this. I'm not opposed to changes here (the current proto package name is especially unfortunate), but I think it's tricky, especially because of the interop concerns.

[chalin]

@dfawley - now that #3546 has been merged, can we reopen this to continue the discussion?

I'd like to start by addressing what I believe is one of your main concerns (mentioned in #3547 (comment)):

... I think it's tricky, especially because of the interop concerns.

By "interop concerns" I suppose that you mean interop tests -- which are run as part of CI tests off of the main grpc/grpc repo? If so, I checked with another member of the gRPC development team and he says that package renaming will not impact interop tests.

[chalin]

I think that we can agree that some renaming is in order. Let's look at some examples along the lines of what I am proposing.

1. OpenTelemetry Go example

The organisation of files for their Hello World example is as follows:

opentelemetry-go/example/grpc/:

• api/
  • hello-service.proto: declares package api
  • hello-service.pb.go
• client/
• server/. Both the client and server use:
  import "go.opentelemetry.io/otel/example/grpc/api"

I would prefer a name like rpc or grpc (which are names I've seen used in other projects), rather than api, which seems too generic IMHO.

demo-grpc is another example that uses the same naming conventions.

2. Opencensus Go example

The organisation of files is as follows:

opencensus-go/examples/grpc/:

• helloworld_client/
• helloworld_server/. Both the client and server use:
  import "go.opencensus.io/examples/grpc/proto".
• proto/: contains the helloworld.proto and helloworld.pb.go

3. Larger example with two proto files

The example described in Writing a microservice in Golang which communicates over gRPC has two proto files, with the indicated package declarations:

• Internal
  • gRPC
  • proto-files
    • domain
      • repository.proto: containing
        package domain;
option go_package = "foo/internal/gRPC/domain";
    • service
      • repository-service.proto: containing
        package service;
option go_package = "foo/internal/gRPC/service";

Client and server code use import statements like:

import (
	"foo/internal/gRPC/domain"
	"foo/internal/gRPC/service"
	...
)

Thoughts?

@dfawley - if you reopen this PR I'll submit some updates. Maybe we can reach an agreement after another couple of iterations.

[dfawley]

By "interop concerns" I suppose that you mean interop tests -- which are run as part of CI tests off of the main grpc/grpc repo? If so, I checked with another member of the gRPC development team and he says that package renaming will not impact interop tests.

The xds example application (in Go: https://github.com/grpc/grpc-go/tree/master/examples/features/xds) apparently has interop concerns. We believe it is fairly common for users to use the server in one language with the client in another.

cc @ejona86 @markdroth

I think the ideal situation would be if the .proto file was moved to the grpc-proto repo, and the package renamed to grpc.examples.helloworld, but I'm worried that the risks of renaming the package aren't worth the benefits.

[ejona86]

I think it is common for users to use the helloworld of one language with the helloworld of another; after using different languages between client and server is a big feature of gRPC. I'd say most of the basic/getting-started interop problems that people asking questions about are using one of the examples.

For xds we have the first time we are actively encouraging people to mix-and-match languages while going through the example process. I think it represents a clearer "point of no return" where the pain would be even greater if we changed the proto-package name. So if we want to change it, I think it needs to be ~now and before the v1.30 release.

I agree that the ideal package name would be grpc.examples.helloworld. Although that would probably still deserve a comment saying "the grpc is there because this example was developed by the grpc team, not because it is a grpc service."

I think it is probably best to just "live with it." A comment describing what the package is and how to select it would be appropriate though. It would be fine to even mention that grpc.examples.helloworld may be a more appropriate name, but that changing the name would break compatibility with all examples since 2015. New examples should "do it right," so we should watch out for this in the one @menghanl is working on.

For me, the strongest argument for fixing the package is that the pain will be a one-time cost. The problem for me is that it seems most of the gain can be had by just having a comment. After a comment is added, the only real issue the current package will cause is that it will look ugly in the grpc-proto repo.

We should go ahead and add the examples to grpc-proto though. I've spoken with @chalin and he'll send out some PRs for this.

[chalin]

Done: grpc/grpc-proto#80