Skip to content
Snippets Groups Projects

WIP: Maint 1.1 bug30905 key values count

Closed WIP: Maint 1.1 bug30905 key values count
maint-1.1_bug30905_KeyValues_count into maint-1.1
3 unresolved threads
Closed juga requested to merge maint-1.1_bug30905_KeyValues_count into maint-1.1
3 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • juga changed milestone to %1.1.x-final

    changed milestone to %1.1.x-final

    By juga on 2020-03-24T07:09:59 (imported from GitLab project)

    • juga
      juga @juga
      Author Owner

      Okay. Here come my review comments (sending them via email to Gitlab seems to be not working yet, so, I do it the old school way via the GUI :)

      Overall this looks like really nice work. Before I will start covering the things I found commit by commit some general remarks:

      1. I am not familiar with how larger patches/patch sets are usually applied to the master branch. I am used to have commits squashed per bug (ideally). And then one commit (rebased on master if needed) merged. I am fine have a bunch of commits merged to master, too.

      2. I am a big fan of each commit passing the tests. This is currently not the case (as you noted). If we do not follow the one commit per bug rule of thumb let's try at least to follow the tests pass per commit one? (I make sometimes suggestions to that below)

      Alright, now the feedback per commit (please let me know if things are unclear):

      89efc2b6722c71b009ef3746a613b9fa512f01a8

      -okay

      fe827ac182d7cee0061660d32dd56a7d11af3c65

      -okay (but change two last two commits; first code change and then test otherwise tests are failing)

      c37eda5169eb3e5aea77331c9af5ad0807326d69

      -(typo commit message: s/since state use json and json will raise error/Since state uses json and json will raise an error/)

      d59af7c90449dc3ea2ae3d466d08066d4d90fb68

      -why does count() not give "1" back if you don't have lists instead of the item itself? (we give essentially the sum back in the list of list case and in the non-list case but not in the list case where we count the amount of members; yeah but that's why the generic count() :))

      ada98e376d6778a7745de9dc89731b125fe58b33

      -s/also an state file/also a state file/

      -s/compossed by/composed of/g

      -s/optionally and state file/optionally a state file/

      -measurements_period is never used, why do we add it now?

      -s/and manage/and manages/ (Should that be "Store" and "manage" or "Stores" and "manages", see different classes)

      -s/an state file/a state file/

      -s/maxlen=self._maxlen,/maxlen=self._maxlen/

      -s/tuples to list/tuples to lists/

      -s/comparations/comparisons/

      -comments in test_update_datetime_int_seq: the first two are copied verbatim from test_update_datetime_seq which does not really fit; s/added, add/added add/

      757ecaf3265c1d47ede54fe9e3fa9a4613523729

      -super(CustomEncoder, self).default(obj), why not just super().default(obj)?

      -do we really need:

         +    d1 = json.loads(s, cls=CustomDecoder)
   +    assert d == d1

      How could they not be the same given the previous assert?

      43a1d661294b81a43cc42d8da85410d38c1e56b5

      -okay

      5da4d63bfe31efeb8bc39d7ce799ab9dc166c2bb

      -okay

      836af37302e88d5aa67ca1e2ef9111347bd42a0e

      -"# This state has recent_consensus_count" <- seems to be wrong

      -please make the test pass at this point (either by changing the assert or the item in the state file)

      -s/to don't have/to not have/

      a53329ad18b7b5058b34c77df643978cce85e213

      -in commit message: s/to to recent_consensus/to recent_consensus/

      -s/bandiwdth/bandwidth/

      -could we save us some resources by avoiding calling state.count() twice:

         +        if state.count("recent_consensus"):
   +            return str(state.count("recent_consensus"))

      like

         count = state.count("recent_consensus")
   if count:
      return str(count)
   else:
      return None

      -s/to don't have/to not have/

      8fe79042668a415546ef3c0cfc5634117f9791af

      -s/to to recent_consensus/to recent_measurement_attempt/

      -s/count consensuses/count relay prioritizations/

      -# As of 2200 what is "2200"? maybe year 2020?

      -# 36000 <- should that be 400 x 120 = 48000?

      -s/grow foreever/go on forever/

      -s/It also test/It also tests/

      -s/state files is updated/state file is updated/

      -s/forth/fourth/

      -s/to don't have/to not have/

      80233c9fbd53aa1243e877a93ea29584b60a12ff

      -s/to to/to/g

      -s/36000, self._state/48000, self._state/ (see previous commit's notes)

      -s/do not grow forever/does not go on forever/

      -s/forth/fourth/g

      -s/the priority lists/the number of relays in the priority lists/ (for increment_priority_relay test)

      -s/only counts the number of priority lists/only counts number of relays in priority lists/ (for increment_priority_relay_test)

      -s/now removed and not counted/now removed and the relays are not counted/ (for incremental priority_relay_test)

      -"# This state has recent_priority_list_count" seems to be wrong (relay count)

      eb68ddf62835fbbe2f2c7fb3039123ae07255fb8

      -okay

      3f9abd5172e79a683466fc73540240044f9cae37

      -okay

      4b09a233936f2adf670cd5b7a7f145129f646106

      -okay

      6f669090ccad62b84d4605a01a1b9d6fe850e10b

      -okay

      13c46faa882472f4a916312c73fc2a082444822f

      -s/grow foreever/go on forever/

      -s/forth/fourth/g

      -comments for test_increment_relay_priority_list() need update (no measurement attempts here)

      4c2e03c453d98c234b809a1ff04e5c6a648d95ec

      -okay, but if we keep a bunch of commits and no single squashed commit then this change should get into the previous commit, though

      1bfd036f078b6d3e68a921eab125fa9d90610ab1

      -okay, but if we keep a bunch of commits and no single squashed commit then this change should get into 13c46faa882472f4a916312c73fc2a082444822f, too

      3a9d1c4cbc5b39a8c811eb372c04bd35135d0af7

      -okay

      6fc4542d7d630c461b3a8a00488556933dedc53b

      -s/ResulSuccess/ResultSuccess/g

      151cfdcc1714740bc081e46c05be3baceb2302ab

      -s/tes net tmp dir/test net tmp dir/

      c0811dd46163a3648a142e5abfa4672c1f476533

      -s/Banwidth file path/Bandwidth file path/

      -s/# 36000/# 48000/

      -what is the reason for the string concatenation in REPORT_TEMPLATE_BWHEADER? Possible line length issues for the later values?

      -why is it always "< max" and not "<= max", e.g. why can't recent_consensus_count be 120 (in fact it actually can be creating a failure when run the script)?

      -we don't use the REPORT_TEMPLATE_BWLINE part yet; could we just remove it in this case? or we add some unit test at least for it to make sure this part is right. it seems the report() call is not working if i do something like

         for line in bwfilw.bwlines:
       line.report()

      But maybe I am misunderstanding something...

      -maybe switch is_measurement_attempt_gte_min() with is_measurement_attempt_lt_max` so we always have the lt_max version before the gte_min one

      1aadfd62df5a397808e8cad805d0e1cb3f31b045

      -s/KeyValues call/KeyValues called/

      -I have some questions for the documentation related commit (the last one) as I am not sure I understand all the changes sentences. Maybe we can get back to that one after all the other changes are in good shape?

      5a643ed1cdfd4187df842500987d7e7c5ff85ff4

      -okay

      aeec130443d995fba1c7cf2fee4af099924bacfa

      -okay

      By Georg Koppen on 2020-04-07T15:13:19 (imported from GitLab project)

    • juga
      juga @juga
      Author Owner

      Georg Koppen:

      Georg Koppen commented:

      Okay. Here come my review comments (sending them via email to Gitlab seems to be not working yet, so, I do it the old school way via the GUI :)

      Overall this looks like really nice work. Before I will start covering the things I found commit by commit some general remarks:

      1. I am not familiar with how larger patches/patch sets are usually applied to the master branch. I am used to have commits squashed per bug (ideally). And then one commit (rebased on master if needed) merged. I am fine have a bunch of commits merged to master, too.

      i know that's the rule in some projects, we have not followed in sbws. So far i prefer we don't squash them.

      1. I am a big fan of each commit passing the tests. This is currently not the case (as you noted). If we do not follow the one commit per bug rule of thumb let's try at least to follow the tests pass per commit one? (I make sometimes suggestions to that below)

      Most of the times i do that. Here some tests were not passing just cause i reorganized the commits. The 1st one is a new idea i got, to deliberately leave a test not passing (that was there before) to point out a bug. Not sure yet which could be the disadvantage

      Alright, now the feedback per commit (please let me know if things are unclear):

      89efc2b6722c71b009ef3746a613b9fa512f01a8

      -okay

      fe827ac182d7cee0061660d32dd56a7d11af3c65

      -okay (but change two last two commits; first code change and then test otherwise tests are failing)

      I'd leave it like this cause of what i commented above.

      c37eda5169eb3e5aea77331c9af5ad0807326d69

      -(typo commit message: s/since state use json and json will raise error/Since state uses json and json will raise an error/)

      To fix on rebase.

      d59af7c90449dc3ea2ae3d466d08066d4d90fb68

      -why does count() not give "1" back if you don't have lists instead of the item itself? (we give essentially the sum back in the list of list case and in the non-list case but not in the list case where we count the amount of members; yeah but that's why the generic count() :))

      Hmm, i wanted to cover the case previous to use lists, ie. if instead of a list there's just a number. Since with the following modifications, we don't read old _count, it should not be the case though.

      ada98e376d6778a7745de9dc89731b125fe58b33

      -s/also an state file/also a state file/

      -s/compossed by/composed of/g

      -s/optionally and state file/optionally a state file/

      -measurements_period is never used, why do we add it now?

      you're right, added without thinking cause the signature of the is_old

      -s/and manage/and manages/ (Should that be "Store" and "manage" or "Stores" and "manages", see different classes)

      According to pep-0257, we should use imperative, not 3rd person:

      The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ...".

      So, i've changed both classes to be consistent and use imperative.

      If there're other docstrings usnig 3rd person, let's leave them for #29047.

      -s/an state file/a state file/

      -s/maxlen=self._maxlen,/maxlen=self._maxlen/

      From Python 3.6, it's fine to have a trailing comma, and it produces smaller diffs in case more arguments are added. It's the default style https://black.readthedocs.io/ uses, and that i'd also like to add in #29047.

      -s/tuples to list/tuples to lists/

      -s/comparations/comparisons/

      -comments in test_update_datetime_int_seq: the first two are copied verbatim from test_update_datetime_seq which does not really fit; s/added, add/added add/

      i rephrased that sentence cause i couldn't understand it anymroe ;) Added an extra comment.

      i pushed a fixup with most of these comments.

      757ecaf3265c1d47ede54fe9e3fa9a4613523729

      -super(CustomEncoder, self).default(obj), why not just super().default(obj)?

      just still used to python 2

      -do we really need:

         +    d1 = json.loads(s, cls=CustomDecoder)
   +    assert d == d1

      Don't think so

      How could they not be the same given the previous assert?

      i don't know :) Pushed fixup

      43a1d661294b81a43cc42d8da85410d38c1e56b5

      -okay

      5da4d63bfe31efeb8bc39d7ce799ab9dc166c2bb

      -okay

      836af37302e88d5aa67ca1e2ef9111347bd42a0e

      -"# This state has recent_consensus_count" <- seems to be wrong

      -please make the test pass at this point (either by changing the assert or the item in the state file)

      -s/to don't have/to not have/

      Pushed Fixup changing the assert

      a53329ad18b7b5058b34c77df643978cce85e213

      -in commit message: s/to to recent_consensus/to recent_consensus/

      Will change it when rebasing

      -s/bandiwdth/bandwidth/

      -could we save us some resources by avoiding calling state.count() twice:

         +        if state.count("recent_consensus"):
   +            return str(state.count("recent_consensus"))

      like

         count = state.count("recent_consensus")
   if count:
      return str(count)
   else:
      return None

      -s/to don't have/to not have/

      Fixup

      8fe79042668a415546ef3c0cfc5634117f9791af

      -s/to to recent_consensus/to recent_measurement_attempt/

      Commit message to change on rebase

      -s/count consensuses/count relay prioritizations/

      same

      -# As of 2200 what is "2200"? maybe year 2020?

      yes

      -# 36000 <- should that be 400 x 120 = 48000?

      yup

      -s/grow foreever/go on forever/

      -s/It also test/It also tests/

      -s/state files is updated/state file is updated/

      -s/forth/fourth/

      -s/to don't have/to not have/

      Fixup

      80233c9fbd53aa1243e877a93ea29584b60a12ff

      -s/to to/to/g

      Change on rebase

      -s/36000, self._state/48000, self._state/ (see previous commit's notes)

      Actually, the constant is already in globals.py in this commit, so better use that. We could subtitute the other constant numbers in a new commit

      -s/do not grow forever/does not go on forever/

      -s/forth/fourth/g

      -s/the priority lists/the number of relays in the priority lists/ (for increment_priority_relay test)

      -s/only counts the number of priority lists/only counts number of relays in priority lists/ (for increment_priority_relay_test)

      -s/now removed and not counted/now removed and the relays are not counted/ (for incremental priority_relay_test)

      -"# This state has recent_priority_list_count" seems to be wrong (relay count)

      In this commit there's now recent_priority_list, though not recent_priority_list_count

      eb68ddf62835fbbe2f2c7fb3039123ae07255fb8

      -okay

      3f9abd5172e79a683466fc73540240044f9cae37

      -okay

      4b09a233936f2adf670cd5b7a7f145129f646106

      -okay

      6f669090ccad62b84d4605a01a1b9d6fe850e10b

      -okay

      13c46faa882472f4a916312c73fc2a082444822f

      -s/grow foreever/go on forever/

      -s/forth/fourth/g

      -comments for test_increment_relay_priority_list() need update (no measurement attempts here)

      Fixup

      4c2e03c453d98c234b809a1ff04e5c6a648d95ec

      -okay, but if we keep a bunch of commits and no single squashed commit then this change should get into the previous commit, though

      Ok

      1bfd036f078b6d3e68a921eab125fa9d90610ab1

      -okay, but if we keep a bunch of commits and no single squashed commit then this change should get into 13c46faa882472f4a916312c73fc2a082444822f, too

      Ok

      3a9d1c4cbc5b39a8c811eb372c04bd35135d0af7

      -okay

      6fc4542d7d630c461b3a8a00488556933dedc53b

      -s/ResulSuccess/ResultSuccess/g

      151cfdcc1714740bc081e46c05be3baceb2302ab

      -s/tes net tmp dir/test net tmp dir/

      yeah, my t key is the most stuck one :) Fixup

      c0811dd46163a3648a142e5abfa4672c1f476533

      -s/Banwidth file path/Bandwidth file path/

      -s/# 36000/# 48000/

      -what is the reason for the string concatenation in REPORT_TEMPLATE_BWHEADER? Possible line length issues for the later values?

      just to don't have code lines > 79

      -why is it always "< max" and not "<= max", e.g. why can't recent_consensus_count be 120 (in fact it actually can be creating a failure when run the script)?

      my mistake, yeah, i think all < max should be <=

      -we don't use the REPORT_TEMPLATE_BWLINE part yet; could we just remove it in this case? or we add some unit test at least for it to make sure this part is right. it seems the report() call is not working if i do something like

         for line in bwfilw.bwlines:
       line.report()

      But maybe I am misunderstanding something...

      Fixed

      -maybe switch is_measurement_attempt_gte_min() with is_measurement_attempt_lt_max` so we always have the lt_max version before the gte_min one

      Leaving it for now

      Fixup

      1aadfd62df5a397808e8cad805d0e1cb3f31b045

      -s/KeyValues call/KeyValues called/

      Fixup

      -I have some questions for the documentation related commit (the last one) as I am not sure I understand all the changes sentences. Maybe we can get back to that one after all the other changes are in good shape?

      Ok

      5a643ed1cdfd4187df842500987d7e7c5ff85ff4

      -okay

      aeec130443d995fba1c7cf2fee4af099924bacfa

      -okay

      By juga on 2020-04-07T15:13:19 (imported from GitLab project)

    • Please register or sign in to reply
  • juga added 11 commits

    added 11 commits

    • 9bd5dfb1 - fixup! chg: timestamps: Add module to manage datetime sequences
    • c3965576 - fixup! chg: json: Create custom JSON encoder/decoder
    • e301e8b9 - fixup! chg: v3bwfile: Convert datetime to str
    • c0756bcc - fixup! chg: relaylist, v3bwfile: Count consensus with timestamps
    • bb5560a4 - fixup! chg: relaylist: Count measurements with timestamps
    • 63c7c9c8 - fixup! chg: relayprioritizer: Count priorities with timestamps
    • f1350dda - fixup! fix: relaylist: Count recent relay's monitoring numbers
    • af5bd7fa - fixup! fix: tests: Add results incrementing relays'
    • 01ae39ae - fixup! fix: tests: Check the files generated in test net
    • 4139d5da - fixup! chg: bwfile: Test KeyValues in a bandwidth file
    • aa288f23 - fixup! fix: doc: Explain changes in the previous commits

    Compare with previous version

    By juga on 2020-04-07T15:01:26 (imported from GitLab project)

    Toggle commit list
  • juga marked as a Work In Progress from sbws@9bd5dfb1

    marked as a Work In Progress from sbws@9bd5dfb1

    By juga on 2020-04-07T15:01:26 (imported from GitLab project)

  • juga
    juga @juga started a thread on commit 63c7c9c8
tests/unit/lib/test_v3bwfile.py
555 555
556 556
557 557 def test_recent_priority_list_count(root_data_path, datadir):
558 # This state has recent_priority_list_count
558 # This state has recent_priority_list
  • juga
    juga @juga
    Author Owner

    I actually think this comment was right. The wrong comment has been in test_recent_priority_relay_count, sorry for the confusion.

    By Georg Koppen on 2020-04-09T05:49:54 (imported from GitLab project)

  • juga
    juga @juga
    Author Owner

    ah, fixup. i should have remove the _count part in any case, but leaving it in the other functions for now, to don't have to push more fixups.

    By juga on 2020-04-09T15:53:11 (imported from GitLab project)

  • Please register or sign in to reply
  • juga
    juga @juga started a thread on commit 01ae39ae
    • tests/integration/conftest.py
    44 44
    45 45 @pytest.fixture(scope='session')
    46 46 def sbwshome_dir(sbwshome_empty):
    47 """Create sbws home inside of the tes net tmp dir without initializing."""
    47 """Create sbws home inside of the test net tmp dir without initializing."""
    48 48 os.makedirs(os.path.join(sbwshome_empty, 'datadir'), exist_ok=True)
    49 49 return sbwshome_empty
    50 50
    • juga
      juga @juga
      Author Owner

      The changes look good. One thing I overlooked in my first round of review was to fix the commit message for 151cfdcc: s/in the the test/in the test/. Please do that when squashing the fixup commits.

      By Georg Koppen on 2020-04-09T05:57:25 (imported from GitLab project)

    • Please register or sign in to reply
  • juga
    juga @juga
    Author Owner

    squashed in !3 (closed)

    By juga on 2020-04-09T15:55:39 (imported from GitLab project)

  • juga closed

    closed

    By juga on 2020-04-09T15:55:39 (imported from GitLab project)

  • juga
    juga @juga
    Author Owner

    I added an additional fixup to make linter pass

    By juga on 2020-04-09T15:58:03 (imported from GitLab project)

  • juga
    juga @juga
    Author Owner

    Reopening to see if gl detect new branch commits

    By juga on 2020-04-10T06:25:32 (imported from GitLab project)

  • juga reopened

    reopened

    By juga on 2020-04-10T06:25:33 (imported from GitLab project)

  • juga
    juga @juga
    Author Owner

    And it does in the list of commits: https://gitlab.torproject.org/torproject/network-health/sbws/-/merge_requests/1/commits, though it doesn't show it here

    By juga on 2020-04-10T06:26:51 (imported from GitLab project)

  • juga closed

    closed

    By juga on 2020-04-10T06:26:52 (imported from GitLab project)

    • Please register or sign in to reply