messages: MessageAckKeyspaceIds

[sougou]

There is a need for a V2 level API for message acks.

BUG=38423920

[sougou]

@michael-berlin I need to override codeclimate for some var names it's flagged. The names are non-compliant because they need to match the proto names. I tried to assert them as WontFix, but it says I'm not authorized.

[michael-berlin]

@michael-berlin I need to override codeclimate for some var names it's flagged. The names are non-compliant because they need to match the proto names. I tried to assert them as WontFix, but it says I'm not authorized.

I think it only complains about the loop variable? You could rename these? ;) Fixing this now (at the price of breaking consistency) would be the easiest solution. This way you also avoid that somebody else runs into this problem at a later point when they touch that code. Note that the golang team doesn't provide directives to exempt something e.g. // NOLINT does not work.

In any case, the Code Climate check is configured as optional (same as with Travis) i.e. you can always override/ignore it. Only the CLA check or the pull approve one require admin rights to ignore it.

[sougou]

I could, but I'd be breaking consistency on how proto names variables. In general, they say that one should not fix golint warnings for golint's sake. Exceptions are allowed where it makes sense.

I can rename these variables, but we'll need a way to override these because there will be cases where it won't make sense to comply.

[michael-berlin]

I could, but I'd be breaking consistency on how proto names variables. In general, they say that one should not fix golint warnings for golint's sake. Exceptions are allowed where it makes sense.

Is this written down somewhere? :) Given that they don't provide a directive to exempt things, I feel like their primary goal is to not have such exceptions in the first place i.e. always write compliant code :)

I can rename these variables, but we'll need a way to override these because there will be cases where it won't make sense to comply.

We can skip the check on a per file basis and do so for generated code: https://github.com/youtube/vitess/blob/master/.codeclimate.yml#L14

For not generated code, I don't want to skip files. Otherwise, we would skip legitimate problems in the same file as well.

[sougou]

Skipping files would not be a good idea. We should just be able to override specific reports (which codeclimate allows, with the right permissions).

The golint documentation specifically states that warnings can be ignored if it doesn't make sense: https://github.com/golang/lint#purpose. And here are some supporting arguments in issues:
golang/lint#89
golang/lint#209

[michael-berlin]

Skipping files would not be a good idea. We should just be able to override specific reports (which codeclimate allows, with the right permissions).

I tried the "Approve" feature in Code Climate and it works for me. Because of my approval, the check is green now.

Did you login with your GitHub account on Code Climate?

I checked the Code Climate settings but didn't find anything which would allow me to configure a list of allowed approvers. My guess is that, similar to Travis, you just need to be a collaborator of the project and then it works? We can follow up with them on that.

Nonetheless, let me repeat what I said earlier:

• you can ignore the check anyway because it's optional
• in my personal opinion, you should just rename the local variables to *ID. Everybody knows that golint is picky about ID and would understand if you break the consistency there. IMO that's better than being bothered by this check over and over again. After all, our Git commit hook will nag you about that as well - and that cannot be permanently ignored like Code Climate seems to support. And unlike generated code, we can change the name here and make the warning go away.

[alainjobart]

One comment on proto file.

[alainjobart]

The keyspace is not optional in v2, is it?

[sougou]

All review comments addressed including variable renaming.