Conversation
| This argument is mutually exclusive with providing 10000 a | ||
| transport instance to ``transport``; doing so will raise | ||
| an exception. | ||
| client_config (dict): DEPRECATED. A dictionary of call options for |
There was a problem hiding this comment.
Is client_config no longer deprecated?
There was a problem hiding this comment.
We no longer want to deprecate it because publish retry is a feature available in Java and Node.
There was a problem hiding this comment.
Note that this is generated code: we don't make direct changes to it, but instead do one of:
- Lobby the
gapic-genrator/gapic-generator-pythonteam to land the desired changes (slow to land / propagate). - Use synth replacement to bash them into the generated module (tedious, painful, and error prone).
Also, this PR is targeting the master branch of your fork, rather than the upstream repo.
There was a problem hiding this comment.
I'm happy to do one or the other. @sduskis which option is better in your opinion?
There was a problem hiding this comment.
There's a third option. We can change publisher/client.py with this logic.
There was a problem hiding this comment.
How would that be possible? As far as I understand how it's set up currently, all retry config settings will be ignored.
What will be my next steps if we make the changes in GAPIC?
There was a problem hiding this comment.
We came up with this idea at a meeting yesterday, but taking a closer look at the code, it seems that we cannot get away just with changing the publisher/client.py, unfortunately - an extra optional argument would indeed be be ignored in gapic PublisherClient in its current implementation.
Modifying the synth.py is thus the way to go. Despite being tedious, it is at least under our own control, making it the least bad option.
There was a problem hiding this comment.
As @tseaver already mentioned, any direct changes here would get wiped out, because this code is autogenerated (and the PR should target the main repository, not the fork :) ).
Had a few comments about the code itself, please take a look. It would also be good to cover it with a few tests.
| for k,v2 in d2.items(): | ||
| v1 = d1.get(k) # returns None if v1 has no value for this key | ||
| if ( isinstance(v1, collections.Mapping) and | ||
| isinstance(v2, collections.Mapping) ): |
There was a problem hiding this comment.
According to the deprecation warning, this will stop working in Python 3.8, and ABCs will have to be imported from collections.abc. Unfortunately, six does not support this (yet?), there is quite some debate on the pull request that would fix that.
DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
It seems that we must import Mapping conditionally (+ a comment to remove it later):
# TODO: remove conditional import after Python 2 compatibility is dropped
if six.PY3:
from collections.abc import Mapping
else:
from collections import MappingThere was a problem hiding this comment.
Thanks for pointing this out!
| Modifies d1 in-place to contain values from d2. If any value | ||
| in d1 is a dictionary (or dict-like), *and* the corresponding | ||
| value in d2 is also a dictionary, then merge them in-place. | ||
| """ |
There was a problem hiding this comment.
I would also explain in the docstring what happens with non-dict values that have the same keys in d1 and d2 (the values from d2 override the ones from d1). Additionally, if this is a module-private helper function, its name should be prefixed with an underscore IMO.
| client_config = default_client_config | ||
| else: | ||
| client_config = publisher_client_config.config | ||
| client_config = merge_dict(default_client_config, client_config) |
There was a problem hiding this comment.
This modifies publisher_client_config.config in-place, potentially affecting any future PublisherClient instances. Merging the dicts should thus be performed on a deep copy of default_client_config.
|
@anguillanneuf, this needs to happen either in |
This PR aims to address this issue.
I have two concerns about this PR:
Function
merge_dictis taken straight from StackOverflow post. Need to contact the user for a license to use the code.Do I need to catch errors with user-supplied config dictionary that may have invalid keys?