nit: this can be `kwargs.get("client_info", DEFAULT_CLIENT_INFO)

1. We should avoid calling this "new", because someday it will be old
2. We don't need internal references for both the gapic client and the veneer data client, since the veneer holds a copy of the gapic.

So we can probably remove _table_data_client and _new_table_data_client, and just have a_data_client, that holds the veneer.

Then indef table_data_client(self):, returnself._data_client._gapic_client

it makes sense to make a lazy-loader for this, but I don't think we want to expose this publicly

I don't think that's a goal. We should keep returning the gapic client, to keep things consistent (although I wouldn't actually expect many users to use the gapic data cleint direclty)

add a comment above this. Maybe something like# client_info argument intended for internal use only, to support legacy client shim

I wonder if we want to set a different user agent for the shim... We'll probably want to know how many people are using the shim, to tell us when it can be removed

Maybe that means we should build our own client_info in the legacy constructor, and only attach this version string if none was passed in?

if "_client_info" in kwargs:  # use client_info passed in from legacy client  self.client_info = kwargs["_client_info"]else:  # set up client info headers for veneer library  self.client_info = DEFAULT_CLIENT_INFO  self.client_info.client_library_version = self._client_version()

Do you need to mock this? I think that was done in other tests as a crude way of disabling background refresh, but maybe that's not needed with the new argument

Another thought: We should use a leading underscore for these undocumented kwargs, to make it extra clear that users shouldn't use them if they find them in the code

nit: havingtable_data_client and_data_client is a little weird. Since this is just for internal use, maybe we should use_veneer_data_client to be more specific?

I was thinking we should customize theclient_library_versionlike in the veneer client, so we can specifically see which requests are going through the shim. Maybe `f"{google.cloud.bigtable.version}-shim"?

What do you think? (Maybe ping Mattie?)

Why do you needif self._instance? It doesn't seem optional?

I don't think we'd ever want this to be None, right?

This is missing app_profile_id.

And maybemutation_timeout should be mapped todefault_mutate_rows_operation_timeout? Or maybe the timeout can just be passed in with each request. I guess you can leave that part for when you start looking at myutations

If this is just for customizing the client_version/background refresh, I'd prefer to avoid tracking this extra state.

We're already passing in a client_info object, so I'd say we should set the value in the legacy client before passing it in, and then do something like:

if kwargs.get("_client_info):    self.client_info = kwargs.get("_client_info")else:  self.client_info = DEFAULT_CLIENT_INFO  self.client_info.client_library_version = self._client_version()

I did like the old version, where there was a flag specifically for disabling background refresh. That could be useful for other purposes too, like testing

Enabling _is_legacy_client could come with other side-effects