github-actions[bot] commented on code in PR #41271:
URL: https://github.com/apache/beam/pull/41271#discussion_r4560215024
##########
sdks/python/apache_beam/metrics/metricbase.py:
##########
@@ -82,9 +82,12 @@ def __init__(
self.name = name
self.urn = urn
self.labels = labels if labels else {}
+ self._cached_fast_name: Optional[str] = None
+ self._cached_hash: Optional[int] = None
+ self._typed_metric_names: dict = {}
Review Comment:
**🟡 Medium**: `self._typed_metric_names` is initialized here and in
`__setstate__` (line 129), and `_TypedMetricName` is mentioned in the PR title,
but `_typed_metric_names` is never read or populated anywhere in the codebase
in this PR. Unless the `MetricsContainer` changes from #41275 are included
here, this allocates an unused empty `dict` on every `MetricName` instance and
should be moved to the PR that consumes it.
##########
sdks/python/apache_beam/metrics/metricbase.py:
##########
@@ -97,17 +100,33 @@ def __str__(self):
self.namespace, self.name)
def __hash__(self):
- return hash((self.namespace, self.name, self.urn) +
- tuple(self.labels.items()))
+ if self._cached_hash is None:
+ self._cached_hash = hash(
+ (self.namespace, self.name, self.urn) +
+ tuple(self.labels.items())) if self.labels else hash(
+ (self.namespace, self.name, self.urn))
+ return self._cached_hash
def fast_name(self):
- name = self.name or ''
- namespace = self.namespace or ''
- urn = self.urn or ''
- labels = ''
- if self.labels:
- labels = '_'.join(['%s=%s' % (k, v) for (k, v) in self.labels.items()])
- return '%d_%s%s%s%s' % (len(name), name, namespace, urn, labels)
+ if self._cached_fast_name is None:
+ name = self.name or ''
+ namespace = self.namespace or ''
+ urn = self.urn or ''
+ labels = ''
+ if self.labels:
+ labels = '_'.join(['%s=%s' % (k, v) for (k, v) in self.labels.items()])
+ self._cached_fast_name = '%d_%s%s%s%s' % (
+ len(name), name, namespace, urn, labels)
+ return self._cached_fast_name
+
+ def __getstate__(self):
+ return (self.namespace, self.name, self.urn, self.labels)
+
+ def __setstate__(self, state):
+ self.namespace, self.name, self.urn, self.labels = state
Review Comment:
**🟡 Medium**: Previously, `MetricName` did not define `__getstate__`, so its
pickled representation was its 4-key instance `__dict__` (`{'namespace': ...,
'name': ..., 'urn': ..., 'labels': ...}`). Because a 4-key `dict` is an
iterable of length 4, unpacking `state` directly when unpickling a
previously-serialized `MetricName` will silently succeed and assign the literal
key strings `'namespace'`, `'name'`, `'urn'`, and `'labels'` as the attribute
values (subsequently crashing in `fast_name()`/`__hash__()` with
`AttributeError: 'str' object has no attribute 'items'`). Handling `dict`
states as well avoids silent data corruption across pickled states.
```suggestion
self.namespace, self.name, self.urn, self.labels = (state['namespace'],
state['name'], state['urn'], state['labels']) if isinstance(state, dict) else
state
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
