Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Select archive #226

Merged
merged 9 commits into from
Jul 5, 2017
Merged

Select archive #226

merged 9 commits into from
Jul 5, 2017

Conversation

adriangalera
Copy link
Contributor

Introduces one new function to override the default fetch algorithm.
In this new algorithm the user can pass the file that he wants to get data from instead of getting the one with the highest precission

@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-2.9%) to 89.132% when pulling d7f4947 on adriangalera:select_archive into 22b1127 on graphite-project:master.

@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-2.9%) to 89.132% when pulling cbe42c8 on adriangalera:select_archive into 22b1127 on graphite-project:master.

@deniszh
Copy link
Member

deniszh commented Jul 3, 2017

Not sure do we need to introduce separate functions for that, dubbing so much code. Maybe you can introduce archiveToSelect=None parameter to both functions?

@DanCech
Copy link
Member

DanCech commented Jul 3, 2017

I agree, there is way too much duplicated code introduced by this patch

@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.7%) to 91.386% when pulling b2f19ed on adriangalera:select_archive into 22b1127 on graphite-project:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 91.386% when pulling b2f19ed on adriangalera:select_archive into 22b1127 on graphite-project:master.

@coveralls
Copy link

coveralls commented Jul 3, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.7%) to 91.386% when pulling aae6e53 on adriangalera:select_archive into 22b1127 on graphite-project:master.

@adriangalera
Copy link
Contributor Author

Implemented the suggested changes in the last commit

deniszh
deniszh reviewed Jul 3, 2017
View reviewed changes
whisper.py Outdated
@@ -872,23 +872,31 @@ def info(path):
return None


def fetch(path, fromTime, untilTime=None, now=None):
def fetch(path, fromTime, untilTime=None, now=None, archiveToSelect=None):
"""fetch(path,fromTime,untilTime=None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change docstring also, please

DanCech
DanCech reviewed Jul 3, 2017
View reviewed changes
whisper.py
@@ -920,13 +928,21 @@ def file_fetch(fh, fromTime, untilTime, now=None):
untilTime = now

diff = now - fromTime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking much better, this line could now be inside the if block.

@coveralls
Copy link

coveralls commented Jul 4, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 91.659% when pulling 6be7c00 on adriangalera:select_archive into 22b1127 on graphite-project:master.

@adriangalera
Copy link
Contributor Author

Implemented the suggested changes in the last commit

DanCech
DanCech requested changes Jul 5, 2017
View reviewed changes
Copy link
Member

@DanCech DanCech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look good, I'd like to see some tests for the archiveToSelect parameter before we merge it.

whisper.py Outdated

#Parse granularity
if archiveToSelect:
retentionStr = str(archiveToSelect)+":1s"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be slightly more efficient if you specify ":1", I'd also move this down into file_fetch so that the archiveToSelect parameter has the same semantics in both functions.

whisper.py Outdated
if archiveToSelect and archive['secondsPerPoint'] == archiveToSelect:
break
elif archiveToSelect:
archive = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be easier to read as a nested if:

if archiveToSelect:
  if archive['secondsPerPoint'] == archiveToSelect:
    break
  archive = None
else:
  if archive['retention'] >= diff:
    break

@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 92.201% when pulling bcf0ffa on adriangalera:select_archive into 22b1127 on graphite-project:master.

@adriangalera
Copy link
Contributor Author

Last commit implements the changes that you suggest and add a test for fetch with archiveToSelect parameter

DanCech
DanCech reviewed Jul 5, 2017
View reviewed changes
whisper.py Outdated

return __archive_fetch(fh, archive, fromTime, untilTime)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace the blank line removed here, then I think this will be good to merge

DanCech
DanCech approved these changes Jul 5, 2017
View reviewed changes
Copy link
Member

@DanCech DanCech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 92.201% when pulling 8866148 on adriangalera:select_archive into 22b1127 on graphite-project:master.

@DanCech DanCech merged commit aab3951 into graphite-project:master Jul 5, 2017
deniszh added a commit to deniszh/whisper that referenced this pull request Jul 8, 2017
@deniszh
Backport of 'Select archive graphite-project#226' to 1.0.x
99e49f5
deniszh added a commit that referenced this pull request Jul 9, 2017
@deniszh
Merge pull request #228 from deniszh/DZ-archive-1.0.x
50790ad 
Backport of 'Select archive #226' to 1.0.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deniszh deniszh deniszh approved these changes

@DanCech DanCech DanCech approved these changes

Assignees
No one assigned
Labels
needs backport to 1.0.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adriangalera @coveralls @deniszh @DanCech