Bigtable HappyBase enable/disable table should be a no-op

[tswast]

Currently methods which aren't available in Bigtable for HappyBase throw an NotImplementedError. https://github.com/GoogleCloudPlatform/gcloud-python/blob/master/gcloud/bigtable/happybase/connection.py#L392

This will make it more difficult to port applications to and from HBase. These operations should be no-ops (possibly logging a warning when run).

[tseaver]

@dhermes I'll defer to your judgement here: I'm not sure what would be worse for the developer porting an application: having unexpected NotImplementedError exceptions raised, or quasi-silent failures. Maybe we need to have an environment variable which toggles the exceptions on (or off)?

[dhermes]

@tswast I feel like not raising the exception would be misleading for the majority of people. Can you elaborate why you'd prefer the silent failure?

[tswast]

No-op is what the Java client does.

This is especially important for third-party tools that build on top of HBase to work on Cloud Bigtable with minimal modification.

I'm not sure if there are any similar tools that build on HappyBase that would have similar concerns, but ideally it should be as easy as possible to port an application to and from Cloud Bigtable and HappyBase.

@mbrukman Do you have an opinion about this?

[dhermes]

I'm not sure where a missing feature of the backend API falls into "easy as possible". As for minimal modification, they could simply remove calls to disable and enable table

[mbrukman]

Let's first agree in principle on the purpose of having a compatibility library such as the HBase client for Java and the HappyBase client for Python:

• our goal is to be compatible with code that uses HBase — this gets us instant integrations that work with HBase and user code that works with HBase becomes trivially portable
• as such, we should aim to have Bigtable become a drop-in replacement for HBase: code that works with HBase should be working with Bigtable with a tiny amount of configuration changes + code to connect to Bigtable
• we should avoid having to tell users to special-case Bigtable in their code that works with HBase, it should be transparent to the code as to whether it's working with HBase or Bigtable
• in essence, our library should hide the differences between HBase and Bigtable, and thus absorb complexity where possible, rather than exposing the differences and forcing customers to write 2 backend implementations, one for HBase and a separate one for Bigtable

Assuming we agree on the above, I think this argues that we should not throw exceptions, but either implement what we need to do, and make unnecessary functions no-ops whenever possible. Of course, each function/method should be considered on its own merits, what its intended semantics are, etc., but the goal here is not to reimplement the HBase semantics 1:1, but make it easy for code to use HBase and Bigtable transparently, with little to no work from the user.

AIUI, in HBase, before deleting a table or modifying some settings, you have to first disable it. Thus, all HBase-compliant code will always call disable_table() prior to calling delete_table().

I would argue that in this case, we should:

• convert calls to disable_table() and enable_table() to be no-ops rather than throw exceptions
• e.g., always return true to is_enabled()
• drop the warning from drop_table() being called with True for dropping table first
• document the difference that Bigtable tables have no concept of disabled state, because we can modify settings and delete tables without the prerequisite of disabling first

Note that this means that we would fail a test of the following type:

...
table.enable()
assertTrue(table.is_enabled())  # succeeds
table.disable()
assertFalse(table.is_enabled())  # fails

but I would argue this is a corner case and not something anyone should be doing in practice, outside of HBase tests themselves.

Other functions/methods should be considered on a case-by-case basis.

Let's consider the opposite: what do we gain from throwing exceptions? I am not sure.

What is the side effect of throwing exceptions?

Typically, this means that portability is hindered from HBase to Bigtable, leading to several options:

• users will either give up, or
• if they want to persevere, they will write abstraction libraries that separate HBase functions from Bigtable functions, or
• users fork gcloud-python and are unable to upgrade since they made local modifications

None of these are good outcomes, from my perspective, as they are providing a sub-par user experience, and we would be the cause of all of them.

Thus, I would advocate that we try to adapt our API to be functionally matching the intended semantics of HappyBase rather than trying to implement them strictly and throw exceptions where we are incompatible, because that breaks the spirit of the point of having a compatibility library in the first place.

Thoughts?

[dhermes]

Seems reasonable enough. I'll send out a PR.

[dhermes]

@mbrukman What about Connection.compact_table(...), Table.regions() and Table.counter_set(...)?

[mbrukman]

@dhermes:

• Connection.compact_table() is a performance optimization (compacted tables have lower depth, hence reads are faster). Bigtable does compactions on its own schedule, so we can safely ignore this as a noop. In essence, we're saying "sure, consider it done, we've taken care of all compactions for you". No semantics difference here.
• Table.regions(): doesn't look like we can return a reasonable value here: returning an empty list will look like it's broken, returning a single item with the first and last key will look like we always have a single region (which is false, and we don't want any application code to assume this is the case), but we also don't want to return the actual tablet server splits, either, so this may have to remain an exception, at least for now. If any user complains about this, please feel free to send them to me.
• Table.counter_set(): I think we should implement it the same way that HappyBase does, but looks like there was an explicit decision made (per the comments) to avoid this, so I may be missing context here. I see the point that it appears to defeat the purpose of a "counter", which I agree with, but presumably HappyBase users had a good (?) reason for adding this method, and it wouldn't break Bigtable or HappyBase to support it as-is. Who was the original author of this? We should chat about this further.

/cc: @sduskis, @garye, @lesv, @rameshdharan — thoughts?

[dhermes]

Want me to fold the Connection.compact_table() no-op into #1997? I am the original author of the gcloud-python port of HappyBase, did you mean https://github.com/wbolster/happybase/? As for counter_set in gcloud-python, I avoided it because I couldn't see a way to make it atomic. If there is a way, then I'm happy to implement it.

[tseaver]

For Table.counter_set(), see #1549. @mbrukman would CheckAndMutateRow (with an appropriate "row-is-empty" predicate) be atomic across both the check and the mutation?

[mbrukman]

@dhermes: yes, please fold the compact_table() noop into your existing change. I was referring to the original author of the code in gcloud-python, not the upstream HappyBase code.

@tseaver: I have a guess, but will defer to @sduskis and @garye to get a definitive answer on this.

[garye]

@mbrukman @tseaver It seems like a CheckAndMutateRow with a pass_all filter (more explicit than leaving it unset) and a single mutation setting the value would do the trick. The check and mutate steps happen in a transaction so the semantics of counter_set should hold.

[garye]

Nevermind! I guess I was confused by the naming because counter_set seems like it's simply a put. No locking or check-and-mutating necessary. I guess it's just an alias that makes code that reads, writes and resets counters more consistent.

[dhermes]

@garye I'm not sure what the implication is. You're saying the counter_set method should just set the value via MutateRow? The HappyBase on HBase impl. just uses put.

[garye]

@dhermes Yeah exactly, my reading of this is that we should just set the value via MutateRow. Which we'd get if we just delegated to put like HappyBase on HBase, right?

[dhermes]

Right.

[tseaver]

Doesn't making counter_set a blind push, rather than a "set only if does not already have a value" make it racy?

[dhermes]

Since this issue is closed maybe we should move the discussion to the actual #1549.

I originally "disabled" counter_set because it can't actually behave like its siblings counter_inc/dec, but I am happy to re-enable and do the "bad thing" that HappyBase already does.

[mbrukman]

We should emulate counter_set in HappyBase as it does, via put, so that we're semantically compatible. We may debate whether that's a good model, but we should be compatible nonetheless.

In this case, it's simply an alias for put and it has comments that say you shouldn't use it for inc/dec operations. This tells me its use cases are intended to be:

• reset to zero
• start from some known non-zero value

As such, we should just implement it with the same semantics as HappyBase does, and let the user decide if it's appropriate for them.

[dhermes]

Yup. Code written, running tests locally and about to send PR.