YACHT-1054: Introducing filtering for tables which are modified till … #82
Conversation
…last census snapshot. Saving Backup Quality SLI history.
radkomateusz
requested review from
jarekdrabek,
marcin-kolda,
MZatorski and
przemyslaw-jasinski
Pull Request Test Coverage Report for Build 765
💛 - Coveralls |
| @@ -23,3 +24,10 @@ def format_query_results(self, results, snapshot_time): | |||
| "backupLastModified": float(result['f'][7]['v']), | |||
| "xDays": self.x_days} for result in results] | |||
| return formatted_results | |||
|
|
|||
| @staticmethod | |||
| def latency_sli_entry_to_table_reference(table): | |||
There was a problem hiding this comment.
Does this method name has to be so long? It only converts json table to TableReference. What's more I think there is similar method somewhere.
There was a problem hiding this comment.
yes, it's just a simply converter, what's more it is exactly the same as quality_sli_entry_to_table_reference
it was created when I think that latency and quality sli entry has different field names.
But I decided to left it as it is, because I think thet the possible change of quality/latency sli schema should not cause necessity to change something in shared converter for another sli
There was a problem hiding this comment.
Alright, I have only 1 more point, how about renaming method to 'entry_to_table_reference' ?
|
|
||
| def __should_stay_as_sli_violation(self, table): | ||
| try: | ||
| return not self.table_newer_modification_predicate.is_modified_till_last_census_snapshot() |
There was a problem hiding this comment.
I think it should be 'is_modified_since_last_census_snapshot'
src/slo/sli_handlers.py
Outdated
| for x_days in [0, 3, 4, 5, 7] | ||
| ] | ||
| url='/sli/quality', | ||
| params={})) |
There was a problem hiding this comment.
I believe, you don't need to provide params if its empty
| @@ -16,11 +16,3 @@ def query(self): | |||
| logging.info("Executing query: %s", query) | |||
| query_results = self.big_query.execute_query(query) | |||
| return self.query_specification.format_query_results(query_results, self.snapshot_time), self.snapshot_time | |||
|
|
|||
| @staticmethod | |||
| def sli_entry_to_table_reference(table): | |||
There was a problem hiding this comment.
I'm wondering why you moved it from here to both specification duplicating the same code? What was the reason?
There was a problem hiding this comment.
answered in comment under Przemek's comment
| @@ -0,0 +1,98 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
I'm wondering if the name of the file is correct :D
| @@ -0,0 +1,97 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
I would change all 'till' to 'since'
…last census snapshot. Saving Backup Quality SLI history.