Opened 9 years ago

Closed 8 years ago

#510 closed enhancement (complete)

Implement dictionary-type variable size counter

Reported by: stephen Owned by: y-aharen
Priority: medium Milestone: Sprint-20111220
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: Auth statistics
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Part of the task to instrument the authoritative server to collect per-zone data.

Subtickets

Change History (21)

comment:1 Changed 9 years ago by stephen

  • Milestone A-Team-Task-Backlog deleted

Milestone A-Team-Task-Backlog deleted

comment:2 Changed 8 years ago by y-aharen

  • Defect Severity set to N/A
  • Milestone set to New Tasks
  • Owner set to y-aharen
  • Status changed from new to accepted
  • Sub-Project set to DNS

comment:3 Changed 8 years ago by y-aharen

  • Milestone changed from New Tasks to Sprint-20110802

comment:4 Changed 8 years ago by y-aharen

  • Owner changed from y-aharen to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:6 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to y-aharen

I have taken the liberty to commit and push a few indentation fixes, and I
changed the logger initialization call in run_unittests.cc, see commit 657349ae281dcdf737b187d0be2cd7d0e4fa92a7

In general, the code looks OK. I do wonder why we need an implementation of a
dictionary, wasn't a direct std::map enough? (or is it impossible to use the
elements by reference then?)

counter_dict_unittests.cc: if the test fixture doesn't do any setup or cleanup,
and holds no state, it's really not nessecary, and the tests can also be done
with the TEST() macro instead of the TEST_F() macro. In this case, nearly every
test however does the same initial thing (define counters(NUMBER_OF_ITEMS)), so
that could actually go into the fixture.

counter_unittests.cc: perhaps the incrementCounterItem test can also try another
round of inc() calls after the three EXPECT_EQ()'s, then check them again, so
that the tests also make sure they do not return fixed values.

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20110927 to Sprint-20111011

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111011 to Sprint-20111025

comment:9 Changed 8 years ago by y-aharen

  • Status changed from reviewing to accepted

comment:10 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111025 to Sprint-20111108

comment:11 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111108 to Sprint-20111122

comment:12 Changed 8 years ago by shane

  • Feature Depending on Ticket set to Auth statistics

comment:13 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by y-aharen

First, I apologize for late response. And thank you for the reviewing.

Replying to jelte:

I have taken the liberty to commit and push a few indentation fixes, and I
changed the logger initialization call in run_unittests.cc, see commit 657349ae281dcdf737b187d0be2cd7d0e4fa92a7

Thank you. The changes looks OK for me too.

In general, the code looks OK. I do wonder why we need an implementation of a
dictionary, wasn't a direct std::map enough? (or is it impossible to use the
elements by reference then?)

In fact, I haven't implemented a dictionary, just using boost::unordered_map.
I'd like to define a concrete interface. A direct std::map exposes much information about internal implementation, so I decided to define an interface to manipulate a set of counters.

counter_dict_unittests.cc: if the test fixture doesn't do any setup or cleanup,
and holds no state, it's really not nessecary, and the tests can also be done
with the TEST() macro instead of the TEST_F() macro. In this case, nearly every
test however does the same initial thing (define counters(NUMBER_OF_ITEMS)), so
that could actually go into the fixture.

Yes, the tests requires initialization, the fixture is needed. If you have any suggestion, please let me know.

counter_unittests.cc: perhaps the incrementCounterItem test can also try another
round of inc() calls after the three EXPECT_EQ()'s, then check them again, so
that the tests also make sure they do not return fixed values.

I've modified the test as you've suggested. Thank you.

I also made a trivial fix to change the way to define enums, as figured in
coding guidelines.

If the branch is OK, I would like to merge it into master, to begin the
following tasks in the next sprint.

comment:14 Changed 8 years ago by y-aharen

  • Owner changed from y-aharen to UnAssigned
  • Status changed from accepted to reviewing

comment:15 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:16 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to y-aharen

Small note; after addressing review comments, you can assign the ticket back to the reviewer directly, there's no need to set it to unassigned again (in fact, that might cause it to be missed by the original reviewer).

In general, the code looks OK. I do wonder why we need an implementation of a
dictionary, wasn't a direct std::map enough? (or is it impossible to use the
elements by reference then?)

In fact, I haven't implemented a dictionary, just using boost::unordered_map.
I'd like to define a concrete interface. A direct std::map exposes much information about internal implementation, so I decided to define an interface to manipulate a set of counters.

Ok

counter_dict_unittests.cc: if the test fixture doesn't do any setup or cleanup,
and holds no state, it's really not nessecary, and the tests can also be done
with the TEST() macro instead of the TEST_F() macro. In this case, nearly every
test however does the same initial thing (define counters(NUMBER_OF_ITEMS)), so
that could actually go into the fixture.

Yes, the tests requires initialization, the fixture is needed. If you have any suggestion, please let me know.

I suggest we move the definition of the counter variable to the fixture, see my commit
60fd293717cc45323cfb10cf06d5bd264fa083cc into branch trac510

counter_unittests.cc: perhaps the incrementCounterItem test can also try another
round of inc() calls after the three EXPECT_EQ()'s, then check them again, so
that the tests also make sure they do not return fixed values.

I've modified the test as you've suggested. Thank you.

I also made a trivial fix to change the way to define enums, as figured in
coding guidelines.

ok, looks good

If the branch is OK, I would like to merge it into master, to begin the
following tasks in the next sprint.

Yes, please go ahead

comment:17 in reply to: ↑ 16 Changed 8 years ago by y-aharen

  • Owner changed from y-aharen to jelte

Replying to jelte:

Small note; after addressing review comments, you can assign the ticket back to the reviewer directly, there's no need to set it to unassigned again (in fact, that might cause it to be missed by the original reviewer).

I'm sorry for your inconvenience. I'll keep it in mind.

counter_dict_unittests.cc: if the test fixture doesn't do any setup or cleanup,
and holds no state, it's really not nessecary, and the tests can also be done
with the TEST() macro instead of the TEST_F() macro. In this case, nearly every
test however does the same initial thing (define counters(NUMBER_OF_ITEMS)), so
that could actually go into the fixture.

Yes, the tests requires initialization, the fixture is needed. If you have any suggestion, please let me know.

I suggest we move the definition of the counter variable to the fixture, see my commit
60fd293717cc45323cfb10cf06d5bd264fa083cc into branch trac510

Thank you for your suggestion.
CounterTest?.invalidCounterSize does not use an instance of Counter in the fixture, so I moved the test to outside. I also modified tests in counter_dict_unittests.cc like you've figured. Thank you.

If the changes are OK, I'll merge it into master.

comment:18 Changed 8 years ago by jelte

  • Owner changed from jelte to y-aharen

Yes, please go ahead

comment:19 Changed 8 years ago by y-aharen

  • Owner changed from y-aharen to jelte

I've addressed the issue which caused build failure on some buildbots. It requires interface modification.
I'll create another ticket to revisit the interface if necessary.

comment:20 Changed 8 years ago by jelte

  • Owner changed from jelte to y-aharen

Looks ok, please try merge again :)

comment:21 Changed 8 years ago by y-aharen

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged into master. Thank you very much for reviewing!

Note: See TracTickets for help on using tickets.