rohdesamuel commented on a change in pull request #16555:
URL: https://github.com/apache/beam/pull/16555#discussion_r788104377
##########
File path:
sdks/python/apache_beam/runners/interactive/caching/streaming_cache.py
##########
@@ -247,8 +247,11 @@ def __init__(
self._sample_resolution_sec = sample_resolution_sec
self._is_cache_complete = is_cache_complete
+ from apache_beam.runners.interactive import interactive_beam as ib
if cache_dir:
self._cache_dir = cache_dir
+ elif ib.options.specified_cache_dir:
+ self._cache_dir = ib.options.specified_cache_dir
Review comment:
I like to call code like this "spooky action at a distance". In general,
configuration should be passed in through the constructor. This makes it
obvious as to what settings the object needs. Accessing a global variable
within the constructor esp. with an existing cache_dir argument can make it
confusing for maintainers. I recommend moving this logic to where the
StreamingCache and CacheManager are created and the cache_dir parameter is
passed.
##########
File path: sdks/python/apache_beam/runners/interactive/interactive_beam.py
##########
@@ -220,6 +220,24 @@ def display_timezone(self, value):
"""
self._display_timezone = value
+ @property
+ def specified_cache_dir(self):
+ """The cache directory specified by the user.
+
+ Defaults to None.
+ """
+ return self._specified_cache_dir
+
+ @specified_cache_dir.setter
+ def specified_cache_dir(self, value):
+ """Sets the cache directory.
+
+ Defaults to None.
+
+ Example::
+ interactive_beam.options.cache_dir = "gs://"
Review comment:
Please fix: the pipeline option in the example ("cache_dir") does not
match the actual pipeline option ("specified_cache_dir").
--
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]
