Opened 7 years ago

Closed 7 years ago

#2843 closed defect (fixed)

isc.statistics.counters.py shouldn't have DNS-specific information

Reported by: jinmei Owned by: naokikambe
Priority: medium Milestone: Sprint-20130806
Component: statistics Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 4.56 Internal?: no

Description

It should be protocol independent and shouldn't have anything like
this:

    # '_SERVER_' is a special zone name representing an entire
    # count. It doesn't mean a specific zone, but it means an
    # entire count in the server.
    _entire_server = '_SERVER_'
    # zone names are contained under this dirname in the spec file.
    _perzone_prefix = 'zones'

Subtickets

Change History (13)

comment:1 follow-up: Changed 7 years ago by vorner

So, what is the proposed solution?

comment:2 in reply to: ↑ 1 Changed 7 years ago by jinmei

Replying to vorner:

So, what is the proposed solution?

Not a complete idea, but we'd revise the main "counters" module so it
will only consist of application-independent data structures such as
named counters and timers. Application logic such as the concept of
"(DNS) zones" should be provided as a separate module, which will use
the generic framework through some generic parameter.

comment:3 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

comment:4 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed

comment:5 Changed 7 years ago by naokikambe

  • Owner set to naokikambe
  • Status changed from new to accepted

picking

comment:6 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed
  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

Ready for reviewing

counters.py was split into generic information and DNS-specific information. dns.py was newly introduced as the latter part. It inherits counters.py. Then Xfrin/Xfrout/Notifyout import dns.py.

Last edited 7 years ago by naokikambe (previous) (diff)

comment:7 Changed 7 years ago by muks

  • Estimated Difficulty changed from 6 to 2

Re-estimating for review

comment:8 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130806

comment:9 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to naokikambe

Hello

Generally, it looks OK. I have just very minor observations.

First one is, you seem to refer to the __doc__ elements directly. While it is technically true the documentation is stored there, I thought python always tried to hide it little bit. Wouldn't it be better to reference to other docstrigs as „see documentation for isc.statistics.counters“ instead of „see isc.statistics.counters.doc“? Or, is there any reason to include the __doc__ there?

I'm little bit worried about the spec file parameter being optional. For one, the general description of both classes says it may be there and how it works if it is there, but it doesn't say what happens when the parameter is not present. But the bigger problem (which was, arguably, not introduced in this ticket) I see is that the hardcoded default is for one single module. Is there a reason to have this default? Wouldn't it be better the spec file was mandatory?

comment:11 in reply to: ↑ 10 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to vorner

Hello, thank you for reviewing.

First one is, you seem to refer to the __doc__ elements directly. While it is technically true the documentation is stored there, I thought python always tried to hide it little bit. Wouldn't it be better to reference to other docstrigs as „see documentation for isc.statistics.counters“ instead of „see isc.statistics.counters.doc“? Or, is there any reason to include the __doc__ there?

I replaced such uses of __doc__. Please see this.

8015b40 [2843] replace __doc__ in documentation with 'documentation for'

I'm little bit worried about the spec file parameter being optional. For one, the general description of both classes says it may be there and how it works if it is there, but it doesn't say what happens when the parameter is not present. But the bigger problem (which was, arguably, not introduced in this ticket) I see is that the hardcoded default is for one single module. Is there a reason to have this default? Wouldn't it be better the spec file was mandatory?

I'm going to remove _Statistics() on #2883. So I updated documentation a little. Please see

7a6cf13 [2843] update documentation for _Statistics class which will be removed

Regards,

comment:12 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to naokikambe
  • Total Hours changed from 0 to 1.06

Hello

Thank you, this looks good. It's OK to merge.

comment:13 in reply to: ↑ 12 Changed 7 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 1.06 to 4.56

Thank you.
Merging was done and the ticket is being closed.

Note: See TracTickets for help on using tickets.