Skip to content
This repository has been archived by the owner on May 4, 2021. It is now read-only.

Add state file with scanner start time #236

Closed
wants to merge 8 commits into from
Closed

Conversation

pastly
Copy link
Member

@pastly pastly commented Jul 16, 2018

Closes #166.

@juga0
Copy link
Contributor

juga0 commented Jul 17, 2018

Regarding datadir it could be named raw_bw_measurements/results in the fs or something like that.
In the code it could be renamed results_dname, but we can also leave it like this.

@@ -210,7 +190,11 @@ def num_lines(self):

@staticmethod
def generator_started_from_file(conf):
return read_started_ts(conf)
state = State(conf['paths']['state_fname'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just add here a comment (or docstring) about what is supposed to be read here.
Something like "ISO formated timestamp which represents the date and time...."



class State:
_ALLOWED_TYPES = (int, float, str, bool, type(None))
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd add also here a docstring or comment about what is this class for and which format it's supposed to store/read (json)

@juga0
Copy link
Contributor

juga0 commented Jul 17, 2018

i'm wondering how easy would be to inherit from json class and overwrite methods. read/write could add just the lock.
It seems that this could also be reaused for results?.
If it means lot of refactoring, it's probably not worth to change this now.

Copy link
Member Author

@pastly pastly left a comment

Choose a reason for hiding this comment

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

I fold some issues in the tests in addition to needing to acknowledge juga's comments.

attempt_keys = (15983, None, True, -1.2, [], {}, set())
for key in attempt_keys:
try:
del state[False]
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be state[key] not state[False]

for key in d:
state[key] = d[key]
for key in state:
pass
Copy link
Member Author

Choose a reason for hiding this comment

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

This should actually test something. Consider checking the length of the iter, as well as making sure the keys in the iter are the same as the keys in d.

@pastly
Copy link
Member Author

pastly commented Jul 17, 2018

After the above reviews, I plan on:

  • leaving "datadir" alone for now as there is a ticket for it
  • adding the two docstrings @juga0 suggested
  • making the test fixes I suggested
  • not trying to inherit JSON class(es) because
    • while results are stored as JSON, they're stored as one JSON object per line to facilitate easier writing and easier ignoring of malformed result data. This is not compatible with the State class in this branch, and it isn't immediately obvious how I would change the State class to work with this design choice.
    • I skimmed the JSON encoding/decoding source code in cpython and it wasn't immediately obvious what I should reimplement and build upon in order to achieve this syncing with the disk functionality.

@pastly
Copy link
Member Author

pastly commented Jul 23, 2018

Done with the above mentioned changes. If you approve, let me know and I can resolve the conflict in README.md and merge. Or if you want to, you can.

@juga0
Copy link
Contributor

juga0 commented Jul 24, 2018

i tend to over-engineer things and i think this is a bit doing it, but it's already done, so it's fine
Re. README, yes, you can solve the conflict

@pastly
Copy link
Member Author

pastly commented Aug 1, 2018

Merged in ccba34a43cf1860d2792deb5cff20508f5c6d848, thanks for the review @juga0

LIES. It was actually bddbf47

@pastly pastly closed this Aug 1, 2018
@teor2345 teor2345 deleted the state_file_02 branch September 6, 2018 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants