Reserve_ids documentation has a typo, and implementation seems bugged

[aawilson]

I've omitted all the usual fields because I feel they are irrelevant, I hope that is okay. Please refer to the PR here: #36

One basic issue is that the documentation of reserve_ids doesn't match the the typespec. The other issue is that the documentation overall is ambiguous enough that I'm actually not sure of its behavior. I would expect it to sequentially reserve all IDs starting from the provided complete key, up to a count of "num", but since all it passes to the underlying API is [complete_key] * num I'm actually not positive that that's what it's doing, or even what it's exactly doing, without anything further to refer to.

[aawilson]

The more I look at the reserveIds API, the more convinced I am that the behavior at this line is completely wrong, in a way that's so weirdly obvious that I'm having trouble believing that I'm not mistaken. Can someone confirm whether or not that is the case? Just to note:

>>> key=c.key('User',111)
>>> complete_key=c.key('User',111)
>>> complete_key_pb = complete_key.to_protobuf()
>>> complete_key_pbs = [complete_key_pb] * 5
>>> complete_key_pbs
[partition_id {
  project_id: "olark-staging"
  namespace_id: "site"
}
path {
  kind: "User"
  id: 111
}
, partition_id {
  project_id: "olark-staging"
  namespace_id: "site"
}
path {
  kind: "User"
  id: 111
}
, partition_id {
  project_id: "olark-staging"
  namespace_id: "site"
}
path {
  kind: "User"
  id: 111
}
, partition_id {
  project_id: "olark-staging"
  namespace_id: "site"
}
path {
  kind: "User"
  id: 111
}
, partition_id {
  project_id: "olark-staging"
  namespace_id: "site"
}
path {
  kind: "User"
  id: 111
}
]
>>>   

[aawilson]

It seems to me like the client method should accept a list of complete keys, not a single "basis" key, and should not accept a "num" argument at all.

[aawilson]

@IlyaFaer I've updated the issue with some additional concerns that are not just centered around documentation. I think this implementation is wrong but cannot confirm the intended behavior because I don't have any special insight into GCD's behavior.

[aawilson]

Pinging this since it's been a month.

[aawilson]

@tseaver Hello! Thanks for accepting the documentation patch, but I think there is more to this issue. Could someone from the team please have a look at #37 (comment) and tell me if I'm crazy or not?

[aawilson]

tl;dr: I'm pretty sure it's constructing a request with a list of ids like

[(Kind, start_id), (Kind, start_id), ...]  # to length num

when it should be like

[(Kind, start_id), (Kind, start_id + 1), ... (Kind, start_id + num - 1)]

Am I reading it wrong, or is this bugged?

[tseaver]

OK, looking at Client.reserve_ids, I agree that the implementation is problematic (it should be bumping the numeric key for each subsequent key created). For backward compatibility, we should:

• Rename the current implementation to reserve_ids_sequential, and fix it to create the correct numeric values.
• Make reserve_ids a deprecated alias for reserve_ids_sequential.
• Add a new method, reserve_ids_multiple (name?), which just takes a sequence of complete keys.