-
Notifications
You must be signed in to change notification settings - Fork 121
Conversation
Venkat2811
commented
Nov 7, 2016
•
edited
Loading
edited
- Abstract Filter class
- WindowingScheme class.
- RangeFilter with and without Windowing scheme
:param upper_bound: Upper bound | ||
:param window_size_sec: Configurable time window for heartbeat | ||
""" | ||
RangeFilter.__init__(self, filter_type, lower_bound, upper_bound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Super
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With base classes __init()__
having different signature, it is not possible to use super()
in derived class. Reference
- One approach is to design base classes for multiple inheritance. Reference. Possible use of kwargs
- Other way is to use
BaseClass.__init__()
So, I thought of using this approach. Your opinion on this please ?
Sets next time-window. | ||
:return: None | ||
""" | ||
self.next_window_time = getUTCmillis() + (self.window_size_sec * 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using getUTCmillis for the first call is fine. Bug getUTCmillis() will return a new value every time, which will be different from the end time of previous window. Hence, the window size will not turn out to be exact as defined by the user, and under heavy loads, the numbers might very heavily. Should we not add the window_size to current window's end time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we need to keep the track of current`s windows end time and on basis of that get the new window end time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Yes, it's a bug :)
from numbers import Number | ||
from aenum import UniqueEnum | ||
from liota.lib.utilities.filters.filter import Filter | ||
import logging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect import order, use pep8 conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outdated version. Please refer the latest one.
A simple lightweight filter, that filters values based on the specified filter type (range). | ||
""" | ||
|
||
def __init__(self, filter_type=None, lower_bound=None, upper_bound=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a default value for filter_type as None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Default value is not necessary. Will change it.
|
||
# --------------------------------------------------------------------------- | ||
# In this example, we demonstrate how System health and some simluated data | ||
# can be directed to two data center components (IoTCC and graphite) using Liota. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment does not seem relevant, we are sending data only to IoTCC in this case. In general, check all comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
@@ -0,0 +1,67 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe in spite of adding this as the separate filter it should happen in the example code. In the UDM both filters(Window as well as value filter) should be applied to one function, which will be more elaborative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll discuss it
# THE POSSIBILITY OF SUCH DAMAGE. # | ||
# ----------------------------------------------------------------------------# | ||
|
||
from abc import abstractmethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an example only for windows filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship It!!!
Waiting for one more review before merging the changes.
Also, if time permits please add the test cases for this pull request after the merge in the tests folder.
Sure @KohliDev, that's the plan. |