Opened 7 years ago

Closed 6 years ago

#2883 closed defect (fixed)

discuss/remove class (global) attribute of Counters._statistics

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

Description

In the Python isc.statistics.counters.Counters class, there's a class
attribute named _statistics:

class Counters():
...
    _statistics = _Statistics()

    def __init__(self, spec_file_name=None):

and Counters._statistics effectively works as a global statistics
object shared by all modules of the program. I didn't understand why
it's defined this way yet, but IMO we should avoid such a global
reference point unless there's a very strong reason for it.

I believe no one disagrees global variables are a last resort weapon
and should be avoided in general (I hope I don't have to explain the
reasons here), and this is indeed a global variable in practice.

My first suggestion is to simply make it an instance attribute, and
update the user side code so it won't assume all Counters objects
share the same single statistics data. If this suggestion happens to
be controversial, my secondary suggestion is to discuss why this must
be global, and if the conclusion still holds document it.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed

comment:3 Changed 7 years ago by naokikambe

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

picking

comment:4 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned
  • Status changed from accepted to reviewing

trac2883 can be reviewed.

Such global attributes (_Statistics) have been removed.

comment:5 Changed 7 years ago by naokikambe

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by muks

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

comment:7 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to naokikambe

Hello

I have few minor points.

In the testing spec file, it seems there's a leftover from copy-paste:

    "module_name": "NotifyOutLike",
    "module_description": "XFR in daemon",

The description might need some update.

This code might get simplified, because the spec_file_name is now mandatory:

        self._statistics_data = {}
        self._statistics_spec = []
        if not spec_file_name: return
        # change the default statistics spec
        self._statistics_spec = \
            isc.config.module_spec_from_file(spec_file_name).\
            get_statistics_spec()

I think this part can be removed completely:

        self._statistics_spec = []
        if not spec_file_name: return
        # change the default statistics spec

I don't think it is legal for user to pass None as the spec file name, so in that case we can omit the return condition. And then, the default value of [] will get overwritten right away.

comment:9 in reply to: ↑ 8 Changed 6 years ago by naokikambe

  • Owner changed from naokikambe to vorner

Hello, thank you for reviewing.

Replying to vorner:

In the testing spec file, it seems there's a leftover from copy-paste:

    "module_name": "NotifyOutLike",
    "module_description": "XFR in daemon",

The description might need some update.

I've changed it to another one:

0207a12 [2883] change module_description of a spec file for testing

This code might get simplified, because the spec_file_name is now mandatory:

        self._statistics_data = {}
        self._statistics_spec = []
        if not spec_file_name: return
        # change the default statistics spec
        self._statistics_spec = \
            isc.config.module_spec_from_file(spec_file_name).\
            get_statistics_spec()

I think this part can be removed completely:

        self._statistics_spec = []
        if not spec_file_name: return
        # change the default statistics spec

I don't think it is legal for user to pass None as the spec file name, so in that case we can omit the return condition. And then, the default value of [] will get overwritten right away.

That's right. I've also update documentation a bit and add a test in case of specifying an invalid argument:

8ce213f [2883] None not acceptable for spec_file_name of the Counters class

Regards,

comment:10 Changed 6 years ago by vorner

  • Owner changed from vorner to naokikambe

I think this can be now merged.

Thank you

comment:11 Changed 6 years ago by naokikambe

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 3

I've merged the branch and am closing the ticket.

Thank you.

Note: See TracTickets for help on using tickets.