[jmxfetch] move status & exit files to '/run' #1679
Conversation
yannmh
force-pushed
the
yann/move-jmxfiles-to-utils
branch
3 times, most recently
from
4d82402
to
ced2a19
Compare
| Removes JMX status files | ||
| """ | ||
| try: | ||
| os.remove(os.path.join(cls._get_dir(), cls._STATUS_FILE)) |
There was a problem hiding this comment.
Can you guarantee that if there is no cls._STATUS_FILE there won't be cls._PYTHON_STATUS_FILE ? Otherwise cls._PYTHON_STATUS_FILE may sometimes not be deleted.
|
Just a small comment, it looks way more beauteous ! 🎉 |
yannmh
force-pushed
the
yann/move-jmxfiles-to-utils
branch
from
ced2a19
to
4980004
Compare
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class JMXFiles(object): |
There was a problem hiding this comment.
I don't get the point of writing a class with only class methods inside (at least not in Python :) ). Couldn't there just be static methods. Then if you import it this way : from utils import jmxfiles you could just call jmxfiles.get_status_file_path(). Wouldn't it make more sense.
There was a problem hiding this comment.
The only point that could be taken here, is that bundling the methods in a class is easier for imports and there is much less risk to override an existing method from another module.
There was a problem hiding this comment.
Yesah but if you use from utils import jmxfiles there can't be a conflict with any other module defining a method with the same name. Am I wrong ?
There was a problem hiding this comment.
Indeed but you import everything class in that module (here I agree there is only one so it's ok).
There was a problem hiding this comment.
Under the hood, it works fine both ways anyway. I was under the influence of this video https://www.youtube.com/watch?v=o9pEzgHorH0 I watched a couple of weeks ago when I made that comment (same kind of ironic tone as the "Linux Sucks" conferences), sorry about that :)
|
Just a slight mistake, as soon as fixed we should bee GTM :) |
Use '/run' for JMXFetch status & exit files instead of '/tmp/'. Move more functions to '/utils'.
elafarge
force-pushed
the
yann/move-jmxfiles-to-utils
branch
from
4980004
to
11f8974
Compare
[jmxfetch] move status & exit files to '/run'
Use '/run' for JMXFetch status & exit files instead of '/tmp/'. Move more functions to '/utils'.