Shouldn't Cluster.hdd_bytes and Cluster.ssd_bytes be in a oneof block?

[dhermes]

I'm implementing the Python library for the API and noticed that both hdd_bytes and ssd_bytes can be set on a CreateCluster call (and they persist when calling GetCluster).

Shouldn't the values (in the Cluster message class) be in a oneof block:

  // The maximum HDD storage usage allowed in this cluster, in bytes.
  int64 hdd_bytes = 6;

  // The maximum SSD storage usage allowed in this cluster, in bytes.
  int64 ssd_bytes = 7;

[dhermes]

/cc @tseaver @jgeewax

[jgeewax]

/cc @coryoconnor @carterpage @maxluebbe -- Can a Bigtable cluster have both hdd_bytes and ssd_bytes ? I'd think no, and that these below in a one-of (union) like @dhermes is saying.

[mgarolera]

The cluster api is not currently enabled. There will be some iteration on it. This fields are likely to not be used any time soon, prefer using only default_storage_type instead.

Right now clusters are only created via the UI and a cluster can only be SSD. In the future we plan to enable HDD clusters, and maybe one day mixed HDD and SSD in the same cluster, which would make these fields meaningful.

[jgeewax]

OK.

This leads to a second question. If we were to send a pull request to modify these protos, would it be worthwhile? Or is this "read-only" open source that will have changes blasted out during some sync job?

[dhermes]

Thanks for the reply @mgarolera !

Regarding

The cluster api is not currently enabled.

I have been able to use it (with a user account, not with a service account) when enabling the "Cloud Bigtable Table Admin API" in the APIs console. See googleapis/google-cloud-python#872 for discussion.

---

@jgeewax It doesn't seem we'd want to make a PR for this issue.

[mgarolera]

@jgeewax: Yes, there protos are not yet the authoritative source and a change would be overwritten.
@dhermes: The api shouldn't be enabled for external users. If you are writing tooling to use it, don't use hdd_bytes and ssd_bytes. HDD clusters are coming soon so it would be useful to be able to control the default_storage_type field.

[dhermes]

Thanks @sduskis!