Update health.proto to add objc prefix

[charlottetan]

No description provided.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: charlottetan / name: Charlotte Tan (e7eca81, ce47b47, 31ecf20)

[sampajano]

@markdroth thanks for cc'ing!

LGTM. But @HannahShiSFB Could you also help take a look please? Thanks!

[sampajano]

Thanks for the change! I'll approve first :)

@HannahShiSFB if there's any concerns please let me know afterwards :)

[HannahShiSFB]

I'm not familiar with it, would you clarify why the change and what will be affected?

[charlottetan]

Thanks! The prefix is needed so that I can call the health check from ios grpc clients.

[markdroth]

I assume that this change affects the generated code in objc. I just want to make sure this isn't a breaking change and isn't going to cause any problems later. @HannahShiSFB or @sampajano, please confirm that you've verified that that is the case.

[charlottetan]

If it helps, I need this to add the objc_grpc_library and objc_proto_library, so that I can send a health check request in my ios test when it starts up

[sampajano]

@markdroth Thanks for checking! Yes indeed this would affect generated code. And if there are existing users on this proto they'd need to update the callsites AFAIU.

@charlottetan Could you explain what error you'd see without this change? I'm wondering if this is the only / best way to address your issue, and whether there are solutions that might not involve breaking existing callers.. @HannahShiSFB FYI.

[charlottetan]

Thanks @sampajano, the error I'm getting is:
error: 'third_party/grpc_proto/grpc/health/v1/health.proto' does not have a required 'option objc_class_prefix'. Target //third_party/grpc_proto:health_objc_grpc failed to build

I get this when I try to do:
blaze build //third_party/grpc_proto:health_objc_grpc

[markdroth]

If the objc codegen won't work at all without this option, then it seems like it can't break anyone to add it.

Are we sure that this option is required for objc code generation even in OSS?

[charlottetan]

It seems that way, I'm happy to validate it a different way or try something else but need help figuring out how

[markdroth]

My last question was aimed at @sampajano, who is our expert on the objc ecosystem. :)

[ejona86]

The namespace should really be more precise than gRPC. We assume the proto package is the namespace so are free to reuse message names across packages. At the very least, if we had a v2 proto, we'd want to add 2 or v2 to the namespace, and GRPCV2 has a vastly different meaning than grpc.health.v2. (Looking at the googleapis repo, I do see that objc namespaces are typically extremely abbreviated. But they still handle v2.)

[charlottetan]

I updated it to GRPCV1, also happy to change it if we prefer a different prefix

[sampajano]

If the objc codegen won't work at all without this option, then it seems like it can't break anyone to add it.

Are we sure that this option is required for objc code generation even in OSS?

@markdroth Yeah i think you're right.. I believe objc code won't be generated at all without objc_class_prefix being specified.. So i think we won't break any "existing" users :)

The namespace should really be more precise than gRPC. We assume the proto package is the namespace so are free to reuse message names across packages. At the very least, if we had a v2 proto, we'd want to add 2 or v2 to the namespace, and GRPCV2 has a vastly different meaning than grpc.health.v2. (Looking at the googleapis repo, I do see that objc namespaces are typically extremely abbreviated. But they still handle v2.)

@ejona86 Thanks for bringing this up!

It does look like all the other language namespaces are more specific than GRPCV1 :)

Maybe we should name this one GrpcHealthV1 instead?

@charlottetan How does that sound to you?

[charlottetan]

SGTM, thanks! Updated to GrpcHealthV1

[sampajano]

@charlottetan Thanks for the QUICK update! :) LGTM :)

[charlottetan]

Thanks! I don't have merge permissions, I'll have to rely on someone else to merge it in.