craigrosie commented on a change in pull request #7227: [AIRFLOW-6530] Allow 
Custom Statsd Client
URL: https://github.com/apache/airflow/pull/7227#discussion_r391939968
 
 

 ##########
 File path: airflow/stats.py
 ##########
 @@ -200,10 +200,35 @@ def __init__(self, *args, **kwargs):
                     self.__class__.instance = DummyStatsLogger()
             except (socket.gaierror, ImportError) as e:
                 log.warning("Could not configure StatsClient: %s, using 
DummyStatsLogger instead.", e)
+                self.__class__.instance = DummyStatsLogger()
 
 Review comment:
   I think I found a bug when adding tests for the new functionality. The log 
message above the line I added states that the class used for the statsd client 
should fall back to `DummyStatsLogger` if the defined statsd client class 
cannot be imported, or cannot connect to a statsd instance, but this never 
happens, and `self.__class__.instance` remains set to the default value of 
`None`
   
   For example, if I try to load an invalid custom statsd class using the 
functionality that @envious added, triggering an `ImportError`, when I then try 
and call `Stats.incr()` (which should still work because the underlying class 
has fallen back to `DummyStatsLogger`), I get errors like:
   ```python
   cls = <class 'airflow.stats.Stats'>, name = 'incr'
   
       def __getattr__(cls, name):
   >       return getattr(cls.instance, name)
   E       AttributeError: 'NoneType' object has no attribute 'incr'
   
   airflow/stats.py:188: AttributeError
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to