Opened 7 years ago

Closed 7 years ago

#2225 closed enhancement (fixed)

Implement counters into Xfrout (3/3)

Reported by: naokikambe Owned by: naokikambe
Priority: medium Milestone: Sprint-20130305
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 7 Add Hours to Ticket: 0
Total Hours: 75 Internal?: no

Description

According to StatisticsItems, implement the following statistics items to Xfrout like #2158 and #2222.

ModuleItemin BIND 9 Statistics XML (tree representation under isc/bind/statistics)Description from BIND 9 ManualImplemented in BIND 10?
Xfrout
xfrout.socket.unixdomain.openserver/ssstat/UnixOpenUnix Domain sockets opened successfullyno
xfrout.socket.unixdomain.openfailserver/ssstat/UnixOpenFailUnix Domain sockets open failuresno
xfrout.socket.unixdomain.closeserver/ssstat/UnixCloseUnix Domain sockets closedno
xfrout.socket.unixdomain.bindfailserver/ssstat/UnixBindFailUnix Domain sockets bind failuresno
xfrout.socket.unixdomain.acceptfailserver/ssstat/UnixAcceptFailUnix Domain sockets incoming accept failuresno
xfrout.socket.unixdomain.acceptserver/ssstat/UnixAcceptUnix Domain sockets incoming connections successfully acceptedno
xfrout.socket.unixdomain.senderrserver/ssstat/UnixSendErrUnix Domain sockets send errorsno
xfrout.socket.unixdomain.recverrserver/ssstat/UnixRecvErrUnix Domain sockets receive errorsno

The classes to be revised are: XfroutServer, XfroutCounter(introduced in #2158), XfroutSession. The related unittest and the lettuce test should be also revised. This ticket depends on #2158 and #2222. Most of fundamental implementation has been done in #2158, so this ticket would not be so difficult.

IMO the rest of items for Xfrout in StatisticsItems makes no sense in current xfrout.py: xfrout.socket.ipv(4|6).(tcp|udp).*, xfrout.socket.unixdomain.conn*.

Because Xfrout doesn't use a IP socket. It receives a unix domain socket via b10-auth. Xfrout doesn't connect by using a unix domain socket.

Subtickets

Attachments (1)

0001-2225_statistics_4-Unify-common-code.patch (1.8 KB) - added by muks 7 years ago.

Download all attachments as: .zip

Change History (56)

comment:1 Changed 7 years ago by naokikambe

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

A proposed ChangeLog entry:

 nnn.   [func]          naokikambe
       New statistics items related unixdomain sockets added into Xfrout :
       open, openfail, close, bindfail, acceptfail, accept, senderr, and
       recverr.  Their values can be obtained by invoking "Stats show Xfrout"
       via bindctl while Xfrout is running.
       (Trac #2225, git TBD)

comment:2 in reply to: ↑ description ; follow-up: Changed 7 years ago by jinmei

Replying to naokikambe:

IMO the rest of items for Xfrout in StatisticsItems makes no sense in current xfrout.py: xfrout.socket.ipv(4|6).(tcp|udp).*, xfrout.socket.unixdomain.conn*.

Because Xfrout doesn't use a IP socket. It receives a unix domain socket via b10-auth. Xfrout doesn't connect by using a unix domain socket.

It does use IP (AF_INET*) sockets. Xfrout simply gets file
descriptors of IPv6/IPv4 sockets via a UNIX domain socket.

comment:3 in reply to: ↑ 2 Changed 7 years ago by naokikambe

Replying to jinmei:
From my view, it seems to use an IP socket but doesn't either seem to open or to accept an IP socket. b10-auth seems to do that instead. Is that right?
Nevertheless, the scope of this ticket doesn't need to be changed, I think.:)

comment:4 Changed 7 years ago by naokikambe

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

The branch can be reviewed.

On that branch the new statistics library (lib/python/isc/statistics) is introduced. Then the existent implement (XfroutCounter) in xfrout.py is migrated to that library. And also other module's counters, e.g. b10-xfrin's ones, can be implemented to that library in the future. So there are more code differences on that branch than the previous ticket #2222.

comment:5 Changed 7 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20121120

comment:6 follow-up: Changed 7 years ago by jinmei

Would the result of #2298 affect this branch? Or is this independent from it?

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

I think it would be independent. Because no stats related codes are touched on this branch. But if it affects, we should work on another ticket.

comment:8 Changed 7 years ago by jinmei

I've given a look at it the branch, and my initial impression is that
we should divide it into the generic statistics (python) module and the xfrout
specific part.

comment:9 follow-up: Changed 7 years ago by jinmei

I can't understand the point of the things defined in
statistics/counter.py. Defining a common base class for counters
is probably a good idea, but I don't understand what it buys
with the convoluted trick of _COUNTER and init(). After all,
most of xfrout-specific work is implemented in XfroutCounter,
so it seems to be sufficient if we just use a straightforward mapping
(dict) from a counter name to value.

I'm also concerned about the singleton nature of the counter object.
IMO, a singleton is generally a source of all problems (very
unfriendly with tests to name one) and should be avoided unless
absolutely necessary. Since I don't see the need for the trick, I
don't understand why it's absolutely necessary either.

In any case, it's very awkward to see things like inc_notifyoutv4()
and XfroutCounter are defined in the generic counter module. It
should be defined somewhere else, somewhere more specific to the
relevant module(s). Same sense of comment applies to the module
description. Using some specific module as an example is probably
good to help understand it, but I think the general description should
be independent from a specific module.

One more specific comment:

  • I don't understand what this means:
    These accessors are effective in other module. For example, in case
    that this module `counter.py` is once imported in such a main module
    as b10-xfrout, Regarding the item `notifyoutv4`, the incrementer
    inc_notifyoutv4() can be invoked via other module like notify_out.py,
    which is firstly imported in the main module.
    
    (and I'm not sure if I understand any of the description below this paragraph)

I'm stopping here, as I can't understand some basic part of the branch.

comment:10 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to naokikambe

comment:11 Changed 7 years ago by naokikambe

Replying to jinmei:

I've given a look at it the branch, and my initial impression is that
we should divide it into the generic statistics (python) module and the xfrout
specific part.

Hello,

I've split it into the following two branches:

trac2225_statistics (mainly adding statistics lib):

22d6e85 [2225] add unixdomain socket as statistics spec
7c00e40 [2225] updated docstrings for counter.py
96edf56 [2225] changed the behavior of the counter class
1c6a6aa [2225] introduced new counter classes and implemented unixsocket counters

trac2225_xfrout (mainly revising xfrout):

06fbc34 [2225] added a new scenario which a slave isn't started
9060a05 [2225] added same tests as the above tests
4e5e32c [2225] removed duplicate tests
fd6e410 [2225] corrected the wrong place to count receive errors of a unix socket
e5268cc [2225] added descriptions about unixsocket counters into the manpage
151379a [2225] added checks for statistics unixsocket counters
07c9232 [2225] changed according the changes of the counter class
7991144 [2225] changed the format of the debug when revised getstats command
44acc7d [2225] removed obsoleted classes from xfrout.py and xfrout_test.py
22d6e85 [2225] add unixdomain socket as statistics spec
7c00e40 [2225] updated docstrings for counter.py
96edf56 [2225] changed the behavior of the counter class
1c6a6aa [2225] introduced new counter classes and implemented unixsocket counters

trac2225_xfrout is on trac2225_statistics (Changes 1c6a6aa^..22d6e85 are common between them). And also I've rebased on the latest master.

I'll answer to the latter questions later.

Regards,

comment:12 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to jinmei

Thank you for the comments.

The purposes of my introducing such a statistics library are:

  • to put the counter code in a place, e.g. counter.py
  • to make less dependency between the counter code and the target code, e.g. xfrout.py
  • to simplify implementation of counters by inheriting the parent class
  • to simplify creation of the counters for the target code, e.g. by invoking init()
  • to make coding test codes easier, e.g. by using inc_xxx() and get_xxx()

So I'll try to answer your questions along with these purposes.

Replying to jinmei:

I can't understand the point of the things defined in
statistics/counter.py. Defining a common base class for counters
is probably a good idea, but I don't understand what it buys
with the convoluted trick of _COUNTER and init(). After all,
most of xfrout-specific work is implemented in XfroutCounter,
so it seems to be sufficient if we just use a straightforward mapping
(dict) from a counter name to value.

_COUNTER and init() are used for initialized counters of b10-xfrout.

In the future, I planned to put common codes into the parent class. In #2300, which is unreviewed ticket, I tried to put most of common codes into the parent class as much as possible.

I'm also concerned about the singleton nature of the counter object.
IMO, a singleton is generally a source of all problems (very
unfriendly with tests to name one) and should be avoided unless
absolutely necessary. Since I don't see the need for the trick, I
don't understand why it's absolutely necessary either.

For example, b10-xfrout should walk with having the counters in each library, e.g. notify_out.py. If the target module creates the counters once, then it can use them in such a library without any further creation. So the counters have to be statically implemented and created. If not, multiple instances of the same counter may be created and it may become difficult to keep consistency between the values of them.

In any case, it's very awkward to see things like inc_notifyoutv4()
and XfroutCounter are defined in the generic counter module. It
should be defined somewhere else, somewhere more specific to the
relevant module(s). Same sense of comment applies to the module
description. Using some specific module as an example is probably
good to help understand it, but I think the general description should
be independent from a specific module.

noftify_out.py can be imported in modules other than xfrout.py like zonemgr.py. But in such a module, such counters doesn't need to be used. Because it doesn't either import or initialize them.

One more specific comment:

  • I don't understand what this means:
    These accessors are effective in other module. For example, in case
    that this module `counter.py` is once imported in such a main module
    as b10-xfrout, Regarding the item `notifyoutv4`, the incrementer
    inc_notifyoutv4() can be invoked via other module like notify_out.py,
    which is firstly imported in the main module.
    

Sorry for my poor English ability. I will explain again here by using the actual codes.

The counters such as inc_notifyoutv4() are imported and initialized at b10-xfrout:

30  from isc.statistics import counter
..
112 # setup statistics counter
113 counter.init(SPECFILE_LOCATION)

Then b10-xfrout's counters, e.g. inc_notifyoutv4(), can be called at notify_out.py:

 28 from isc.statistics import counter
..
482             # count notifying by IPv4 or IPv6 for statistics
483             if zone_notify_info.get_socket().family == socket.AF_INET:
484                 counter.inc_notifyoutv4(zone_notify_info.zone_name)

Unless counters are imported and initialized in such a way, counters like inc_notifyoutv4() cannot be workable in notify_out.py. Because just an empty counter is called in notify_out.py, which is directly defined at counter.py:

 94 # These method are dummies for notify_out in case XfroutCounter is not
 95 # loaded.
 96 def inc_notifyoutv4(self, arg):
 97     """An empty method to be disclosed"""
 98     pass

Regards,

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by jinmei

Replying to naokikambe:

I can't understand the point of the things defined in
statistics/counter.py. Defining a common base class for counters
is probably a good idea, but I don't understand what it buys
with the convoluted trick of _COUNTER and init(). After all,
most of xfrout-specific work is implemented in XfroutCounter,
so it seems to be sufficient if we just use a straightforward mapping
(dict) from a counter name to value.

_COUNTER and init() are used for initialized counters of b10-xfrout.

In the future, I planned to put common codes into the parent class. In #2300, which is unreviewed ticket, I tried to put most of common codes into the parent class as much as possible.

In that case I guess we should begin with that version of base class.
Right now, the base class just seems to be a redundant abstraction,
just making the architecture non understandable without any benefit.

I'm also concerned about the singleton nature of the counter object.
IMO, a singleton is generally a source of all problems (very
unfriendly with tests to name one) and should be avoided unless
absolutely necessary. Since I don't see the need for the trick, I
don't understand why it's absolutely necessary either.

For example, b10-xfrout should walk with having the counters in each library, e.g. notify_out.py. If the target module creates the counters once, then it can use them in such a library without any further creation. So the counters have to be statically implemented and created. If not, multiple instances of the same counter may be created and it may become difficult to keep consistency between the values of them.

Maybe I don't fully understand it, but, to me, defining a things like
statistics counters at the module level doesn't seem to be a good idea
in the first place. It looks more reasonable to me if it's composed
in some other class object, such as XfroutServer.

In any case, it's very awkward to see things like inc_notifyoutv4()
and XfroutCounter are defined in the generic counter module. It
should be defined somewhere else, somewhere more specific to the
relevant module(s). Same sense of comment applies to the module
description. Using some specific module as an example is probably
good to help understand it, but I think the general description should
be independent from a specific module.

noftify_out.py can be imported in modules other than xfrout.py like zonemgr.py. But in such a module, such counters doesn't need to be used. Because it doesn't either import or initialize them.

Here we seem to be talking about different issues. I simply said it
looks strange that the module named isc.statistics.counter (which is
even independent from DNS, much less xfr or notify) contains something
specific to "notify" or "xfr". Wherever the actual place XfroutCounter
is defined, it at least shouldn't be in isc.statistics.counter.

According to your plan, the base counter class may contain more logic
that is currently implemented in XfroutCounter. Depending on
whether the resulting base class is a clean abstraction, it may or may
not make sense. So, I again, I suggest we begin with that
abstraction, i.e, define a more meaningful base Counter class, and
shows how xfr can use it module-independent manner.

comment:14 Changed 7 years ago by jinmei

  • Owner changed from jinmei to naokikambe

comment:15 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to jinmei

After I'm given your comments, I've realized as the followings. Is this correct?

  • We may implement an abstract counter class under isc.statistics.
  • We shouldn't implement a concrete counter class, e.g. XfroutCounter, for a specific module under isc.statistics.
  • We may implement such a concrete counter class in the module, e.g. b10-xfrout.py, which uses it.
  • We should complete implementation of an abstract counter class like #2300 in advance.

If correct, I'd like to restart from the fourth one.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by jinmei

Replying to naokikambe:

After I'm given your comments, I've realized as the followings. Is this correct?

In general, yes.

  • We may implement an abstract counter class under isc.statistics.
  • We shouldn't implement a concrete counter class, e.g. XfroutCounter, for a specific module under isc.statistics.

Maybe "shouldn't" sounds too strong, but it just looked too awkward to
me to have such things as XfroutCounter in the isc.statistics package.

  • We may implement such a concrete counter class in the module, e.g. b10-xfrout.py, which uses it.

Or at the very least not directly in the isc.statistics package. We
could either introduce a helper module for xfrout and define its
counter there or implement directly in xfrout. Maybe the former is
better in terms of modularity.

  • We should complete implementation of an abstract counter class like #2300 in advance.

I think so. At least in the current form I cannot be sure whether the
idea of the abstract class is good or not.

In addition, in my opinion I'd avoid implementing counter objects at
the module scope.

BTW: I've given an initial view to the ticket as it's been mostly
forgotten for quite a long time, but I'm afraid I have too many things
on my table now. When you complete it, I think it's better to ask
someone else to give it a full review.

comment:17 Changed 7 years ago by jinmei

  • Owner changed from jinmei to naokikambe

comment:18 in reply to: ↑ 16 Changed 7 years ago by naokikambe

Actually, we cannot help implementing python codes either under src/bin/foo or src/lib/python/isc/foo. So we might like to implement a code for a specific module under src/lib/python. For example, ConfigManager is located under src/lib/python/isc/config, however any other modules than b10-cfgmgr.py and a few test codes don't seem to use it. We have such exceptional cases.

Anyway, I'll try to revise the counter codes along with the above guideline. And then I'll pass to another reviewer. Thank you for giving the comments.

comment:19 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to UnAssigned

The both branches trac2225_statistics and trac2225_xfrout are ready for reviewing. trac2225_statistics(ad4eae2) is only for isc.statistics. trac2225_xfrout(d48e433) is for implementing counters on xfrout by using trac2225_statistics.

In advance I've applied commits which are already applied in #2252, #2274, and #2300. However those commits are only ones related to isc.statistics, isc.notify.notify_out, and b10-xfrout. I've also added other changes for the above guideline.

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

comment:20 follow-up: Changed 7 years ago by jinmei

From a quick look, it's still like a singleton/module-scope object (right?).
Personally, I'm not yet convinced this is the way to go.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by naokikambe

Replying to jinmei:
Sorry, I couldn't understand exactly what singleton is as you mentioned. That is, is it not allowed to return an instance created before? If so, it isn't so difficult to revise to return a newly created instance, I think.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by jinmei

Replying to naokikambe:

Sorry, I couldn't understand exactly what singleton is as you mentioned. That is, is it not allowed to return an instance created before? If so, it isn't so difficult to revise to return a newly created instance, I think.

In that we cannot have multiple xfrout counter objects, for example.
Conceptually I don't see any difficulty in that, because counters are
essentially a map from some ID to integers. If the current singleton
design is to simply allow cool syntax sugar like this:

  Counter.inc_xfrreqdone(zone_name)

I *personally* strongly oppose to the idea.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by naokikambe

Replying to jinmei:
OK, I understand your opinion. So do you have another idea? For example, can xfrout directly update a map-type counter as the following? Is this reasonable for you?

Counter.statistics['zones'][zone_name]['xfrreqdone'] += 1

comment:24 in reply to: ↑ 23 ; follow-up: Changed 7 years ago by jinmei

Replying to naokikambe:

Replying to jinmei:
OK, I understand your opinion. So do you have another idea? For example, can xfrout directly update a map-type counter as the following? Is this reasonable for you?

Counter.statistics['zones'][zone_name]['xfrreqdone'] += 1

The Counter part still seems to indicate singleton. Conceptually,
what I'd envision is this:

    xfrout_counter = some_factory_method_or_something()
    xfrout_counter.inc('zones', zone_name, 'xfrreqdone')

(Note that this is conceptual example. We can consider syntax sugar
to improve readability as long as the essential property is preserved)

comment:25 in reply to: ↑ 24 ; follow-up: Changed 7 years ago by naokikambe

Replying to jinmei:
Your suggestion seems to be more straightforward and safer than mine, I think. The essential property, as you mentioned, is to call such a constructor or other like method before incrementation? If so, I could basically agree with this. So can I restart revising codes? I need some time for that.

comment:26 in reply to: ↑ 25 Changed 7 years ago by naokikambe

I've revised the interface of Counter(). The ways to create a object and call a method are like the following. Arguments of inc() depends on a spec file given at creation of the object.

# create a counter object
self.counter = Counter(SPECFILE_LOCATION)
# increment a counter
self.counter.inc('zones', zone_name, 'xfrreqdone')

I've pushed new codes to support the above. They are ready for reviewing. The branches are trac2225_statistics(0c8094b) and trac2225_xfrout(01c389a). trac2225_statistics is for the statistics library and trac2225_xfrout is for its implementation into xfrout. trac2225_xfrout depends on trac2225_statistics.

Regards,

comment:27 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking for review.

comment:28 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

We decided during the daily status call yesterday that first we'll first review just the trac2225_statistics branch. After it is reviewed and merged, the other branch trac2225_xfrout can be reviewed, either in this ticket or as part of another ticket if required. So, here is my first review of the trac2225_statistics branch.

First, I don't understand if Counter is expected to behave somewhat like a singleton, or I don't follow why some code is written the way it is (the _Statistics class, and some Counter initialization outside __init__()). Is it expected to create equivalent Counter objects, based on what spec file is passed? Suppose that you have the following code:

  from isc.statistics import Counter
  counter1 = Counter("/path/to/foo.spec")
  counter2 = Counter("/path/to/bar.spec")
  counter3 = Counter("/path/to/foo.spec")
  counter1.clear_counters()

Are counter1 and counter3 supposed to be equivalent? They aren't based on the current code, but are they intended to be equivalent? With the last statement in the block of code above, are counters in counter3 also supposed to be cleared? (I think they are not, currently). If such equivalence is desired, it's better to rewrite it such that there is a factory method in the counter module, that returns a Counter object based on the passed spec file.

I am wondering about the above, because a singleton was in use before and I want to know what the intention was.

Assuming that the code is as-is, the following are my comments:

  • I suggest renaming the class to the plural form (Counters) as it manages multiple counters.
  • The module's documentation says that init() must be called. Where is init() defined?
  • Some methods are declared at static scope to the module, rather than as a part of Counter class itself. We can indicate them as private by prefixing an underscore. Is there a reason why Counter.inc() calls _inc_counter() and not self._inc_counter()?
  • Counter class needs documentation (most of the module documentation should be moved here)
  • Counter.__init__() should be documented to indicate what happens when spec_file_name is passed, and in the case where it is None.
  • What is the purpose of Counter._rlock? If it is necessary, shouldn't access to Counter._disabled also be synchronized by using it, in .enable(), .disable(), .inc(), .dec(), etc.?
  • The following repeated code is best moved to a function, so that we have a single place to update if we change how the key is computed from the args.
    	identifier = '/'.join(args)
    
  • If Counter.inc() and .dec() are largely similar, except for the step, they should be unified by moving most of the code to a common function.
  • In _add_counter(), is the try necessary?
        try:
            isc.config.find_spec_part(spec, identifier)
        except isc.cc.data.DataNotFoundError:
            # spec or identifier is wrong
            raise
    
  • Is there a test for the reason why the format string is used here:
        spec_ = isc.config.find_spec_part(
    	spec, '%s' % identifier.split('/')[0])
    
  • The behavior of _set_counter(), _get_counter() and _inc_counter() seem inconsistent when identifier isn't already known. _set_counter() and _inc_counter() both add it if necessary, but _get_counter() doesn't (and throws).
  • If the timer sets the delta time in seconds, isn't it simpler to use time.time() instead of datetime and timedelta? It would also avoid the timedelta to seconds computation in _stop_timer().
  • Counter.stop() documentation should say what units of time (seconds, microseconds, etc.)
  • In Counter.stop(), I suggest changing it so it returns upon isc.cc.data.DataNotFoundError instead of using the else, but it's just a matter of personal style I guess. Please consider it and decide what you prefer best.
  • I suggest renaming Counter.start() to indicate that it is a timer that is started, i.e., Counter.start_timer().
  • I also suggest renaming Counter.clear_counters() to Counter.clear() or .clear_all() (see also the suggestion above about renaming the class to Counters). I don't know if Counter is intended to have any singleton behavior, but if it does, its doc should also be updated to indicate that.
  • The expression below should be commented (I see what it does, but a comment would help a reader quickly understand). You may be able to get this from the args argument itself rather than splitting identifier. Also as I mentioned above, it's best to do such key computation in a separate method so it can be understood and updated easily.
    	        '/'.join(identifier.split('/')[0:-1]))\
                    [identifier.split('/')[-1]]
    
  • In Counter.dump_statistics(), is the comment always correct?
    	# If self.statistics_data contains nothing of zone name, it
    	# returns an empty dict.
    	if self._perzone_prefix not in statistics_data:
                return statistics_data
    
  • Is get_statistics() a better name than dump_statistics() (as it returns a dict)?

Locally I also updated the module documentation to make it more clear, but now I think it's best we address the comments above first. I'll update the resulting docs after that which you can review.

comment:29 in reply to: ↑ 28 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hello, thank you for reviewing.

Replying to muks:

We decided during the daily status call yesterday that first we'll first review just the trac2225_statistics branch. After it is reviewed and merged, the other branch trac2225_xfrout can be reviewed, either in this ticket or as part of another ticket if required. So, here is my first review of the trac2225_statistics branch.

I agreed. Let's focus on reviwing trac2225_statistics.

First, I don't understand if Counter is expected to behave somewhat like a singleton, or I don't follow why some code is written the way it is (the _Statistics class, and some Counter initialization outside __init__()). Is it expected to create equivalent Counter objects, based on what spec file is passed? Suppose that you have the following code:

  from isc.statistics import Counter
  counter1 = Counter("/path/to/foo.spec")
  counter2 = Counter("/path/to/bar.spec")
  counter3 = Counter("/path/to/foo.spec")
  counter1.clear_counters()

Are counter1 and counter3 supposed to be equivalent? They aren't based on the current code, but are they intended to be equivalent? With the last statement in the block of code above, are counters in counter3 also supposed to be cleared? (I think they are not, currently). If such equivalence is desired, it's better to rewrite it such that there is a factory method in the counter module, that returns a Counter object based on the passed spec file.

I am wondering about the above, because a singleton was in use before and I want to know what the intention was.

In one bind10 module, it is NOT indented that the different spec files are read in the above way. Maybe I think counters in counter3 would be cleared by counter1.clear_counters().

However in different modules as followings, it is indented that each spec file is read individually. That is, counter1.clear_counters() doesn't clear counters in counter2, and vice versa.

Module Foo (b10-foo):

from isc.statistics import Counter
counter1 = Counter("/path/to/foo.spec")
counter1.clear_counters()

Module Bar (b10-bar):

from isc.statistics import Counter
counter2 = Counter("/path/to/bar.spec")
counter2.clear_counters()

So if we should consider such equivalence in the above former case, I also think we would introduce a factory method or something as you mentioned. Sorry, please suggest whether we should or not. If yes, I'd like to stop reviewing and try to redesign the interface of the class.

Assuming that the code is as-is, the following are my comments:

  • I suggest renaming the class to the plural form (Counters) as it manages multiple counters.

Ok, I've renamed:

d4e066a [trac2225_statistics] rename the class to the plural form (`Counters`) as it manages multiple counters
  • The module's documentation says that init() must be called. Where is init() defined?

Not defined. I've corrected documentation.

6a4b958 [trac2225_statistics] correct documentation about an undefined method
  • Some methods are declared at static scope to the module, rather than as a part of Counter class itself. We can indicate them as private by prefixing an underscore. Is there a reason why Counter.inc() calls _inc_counter() and not self._inc_counter()?

Because we don't need to relate _inc_counter() with self, I did so. There is no reason other than this.

  • Counter class needs documentation (most of the module documentation should be moved here)

I've added documentation but it might need to be added more.

4b368b5 [trac2225_statistics] correct and add documentation of the Counters class


  • Counter.__init__() should be documented to indicate what happens when spec_file_name is passed, and in the case where it is None.

I've documented about the cases that the spec file is specified and omitted.

dbf92ea [trac2225_statistics] document about cases when a spec file is specified and omitted


  • What is the purpose of Counter._rlock? If it is necessary, shouldn't access to Counter._disabled also be synchronized by using it, in .enable(), .disable(), .inc(), .dec(), etc.?

It is necessary because counters can be incremented by the different threads. So it should as you mentioned. Also a timer cannot be update if it's disabled.

2f7080c [trac2225_statistics] synchronize when accessing self._disabled
  • The following repeated code is best moved to a function, so that we have a single place to update if we change how the key is computed from the args.
    	identifier = '/'.join(args)
    

I've added a helper method _concat() and replaced with it.

301ccaf [trac2225_statistics] add a helper method _concat and its test
  • If Counter.inc() and .dec() are largely similar, except for the step, they should be unified by moving most of the code to a common function.

I've added step in kwarg in inc() and directly used it in dec().

4ae14bd [trac2225_statistics] add step as kwarg into inc() and use it in dec() with step=-1
  • In _add_counter(), is the try necessary?
        try:
            isc.config.find_spec_part(spec, identifier)
        except isc.cc.data.DataNotFoundError:
            # spec or identifier is wrong
            raise
    

No, it was not neccesary. Thanks.

7ec3307 [trac2225_statistics] remove an unnecessary try/expect statement
  • Is there a test for the reason why the format string is used here:
        spec_ = isc.config.find_spec_part(
    	spec, '%s' % identifier.split('/')[0])
    

I've removed the format string, thanks.

0d0de6a [trac2225_statistics] remove an unnecessary formating
  • The behavior of _set_counter(), _get_counter() and _inc_counter() seem inconsistent when identifier isn't already known. _set_counter() and _inc_counter() both add it if necessary, but _get_counter() doesn't (and throws).

Sorry, I couldn't understand why _get_counter() should add. I think _get_counter() should just return the current value and should not change the statistics data. OTOH _set_counter() and _inc_counter() are intended for adding an entry into statistics data if there is not.

_get_counter() is not indended to be used before the counter is incremented. It's mostly for a testing purpose.

  • If the timer sets the delta time in seconds, isn't it simpler to use time.time() instead of datetime and timedelta? It would also avoid the timedelta to seconds computation in _stop_timer().

I decided to use datetime.now() because of the following python reference:
http://docs.python.org/3.2/library/datetime.html#datetime.datetime.now

supplies more precision than can be gotten from going through a time.time() timestamp

But we can use timedelta.total_seconds() instead of such seconds computation. I've revised the code.

868558c [trac2225_statistics] use timedelta.total_seconds() instead of redundant computation
  • Counter.stop() documentation should say what units of time (seconds, microseconds, etc.)

I've updated documentaion.

1f05684 [trac2225_statistics] update document of Counters.stop()
  • In Counter.stop(), I suggest changing it so it returns upon isc.cc.data.DataNotFoundError instead of using the else, but it's just a matter of personal style I guess. Please consider it and decide what you prefer best.

I've removed 'else', thanks.

289118e [trac2225_statistics] remove 'else' statement
  • I suggest renaming Counter.start() to indicate that it is a timer that is started, i.e., Counter.start_timer().

I've renamed start()/stop(), thanks.

db1ad53 [trac2225_statistics] rename start()/stop() to start_timer()/stop_timer()
  • I also suggest renaming Counter.clear_counters() to Counter.clear() or .clear_all() (see also the suggestion above about renaming the class to Counters). I don't know if Counter is intended to have any singleton behavior, but if it does, its doc should also be updated to indicate that.

I've renamed it to clear_all(), thanks. But I left documentation about the above discussion on singleton.

bfb0225 [trac2225_statistics] rename to clear_all()
  • The expression below should be commented (I see what it does, but a comment would help a reader quickly understand). You may be able to get this from the args argument itself rather than splitting identifier. Also as I mentioned above, it's best to do such key computation in a separate method so it can be understood and updated easily.
    	        '/'.join(identifier.split('/')[0:-1]))\
                    [identifier.split('/')[-1]]
    

I've added documents. Is it more helpful?

2aa30dc [trac2225_statistics] add helpful comments to the complicated expression
  • In Counter.dump_statistics(), is the comment always correct?
    	# If self.statistics_data contains nothing of zone name, it
    	# returns an empty dict.
    	if self._perzone_prefix not in statistics_data:
                return statistics_data
    

It's not correct. I've updated.

4688552 [trac2225_statistics] update document as it is
  • Is get_statistics() a better name than dump_statistics() (as it returns a dict)?

I've renamed.

4661800 [trac2225_statistics] rename dump_statistics() to get_statistics()

Locally I also updated the module documentation to make it more clear, but now I think it's best we address the comments above first. I'll update the resulting docs after that which you can review.

Thank you very much! I've also updated documentation above but it might not be enough. Please review again.

Regards,

comment:30 Changed 7 years ago by jelte

  • Milestone changed from Sprint-DHCP-20130103 to Sprint-20130108

comment:31 Changed 7 years ago by muks

Hi Kambe san

I'm re-reviewing this bug now. Sorry it took longer than I expected to attend to other bugs.

comment:32 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

Here are my comments:

  • counter.py should be renamed to counters.py.
  • Multi-module tests should be added for the various scenarios in which the counters of a single spec file will be used, i.e, for cases similar to the notifyoutv4 example (between b10-xfrout and notify_out) given in the counter.py module doc, and also for any other cases where such common counts will be maintained.
  • We should note in __concat()'s documentation that it is a helper function that is used to generate the keys.
  • I think we should remove the step argument from inc() and dec() and move the code to a common helper method. Exposing step to a caller as it is done currently is not good code. I have added a comment temporarily to both methods' docs that step should not be specified by the caller.
  • Is dec() only meant to be used for axfr_running and ixfr_running?
  • This is not a good API doc comment because you are exposing internal implementation details to the user of this module which that user may not know about (in order to follow the comment). It's simpler to just say "Starts a timer and keeps it running until stop_timer() is called.", and also talk about any other things that the caller should know. Same for stop_time()'s API doc too.
            """Sets the value returned from _start_timer() as a value of the
            identifier in the self._start_time which is dict-type"""
    
  • In the following API doc, do you mean returns instead of raises?
            self._start_time after setting is successfully done. In case
            of stopping the timer which has never been started, it raises
            and does nothing. But in case of stopping the time which isn't
    
  • I have pushed some comment updates to the trac2225_statistics branch. Please review them carefully and check if they are ok.

comment:33 in reply to: ↑ 32 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund san

Thanks for the comments.

  • counter.py should be renamed to counters.py.

I renamed.

dc7e03f [2225_statistics] rename counter.py to counters.py and make the related changes for the review comment
  • Multi-module tests should be added for the various scenarios in which the counters of a single spec file will be used, i.e, for cases similar to the notifyoutv4 example (between b10-xfrout and notify_out) given in the counter.py module doc, and also for any other cases where such common counts will be maintained.

I added some tests into counters_test.py. Is that enough? Please read this.

cc0c7a0 [2225_statistics] add multi-module tests for the scenarios
  • We should note in __concat()'s documentation that it is a helper function that is used to generate the keys.

I revised the note.

08d9023 [2225_statistics] add note to _concat()
  • I think we should remove the step argument from inc() and dec() and move the code to a common helper method. Exposing step to a caller as it is done currently is not good code. I have added a comment temporarily to both methods' docs that step should not be specified by the caller.

I added _incdec() as a helper function. Please see the following for details.

78f11bb [2225_statistics] add a common helper function for incrementing or decrementing a counter
  • Is dec() only meant to be used for axfr_running and ixfr_running?

That documentation was incorrect. I revised it.

6443e07 [2225_statistics] correct documentation of inc() and dec()
  • This is not a good API doc comment because you are exposing internal implementation details to the user of this module which that user may not know about (in order to follow the comment). It's simpler to just say "Starts a timer and keeps it running until stop_timer() is called.", and also talk about any other things that the caller should know. Same for stop_time()'s API doc too.
            """Sets the value returned from _start_timer() as a value of the
            identifier in the self._start_time which is dict-type"""
    
  • In the following API doc, do you mean returns instead of raises?
            self._start_time after setting is successfully done. In case
            of stopping the timer which has never been started, it raises
            and does nothing. But in case of stopping the time which isn't
    

Yes, but I've revised documentation for accuracy. Please read this.

927c2a9 [2225_statistics] simplify too detailed documentation in start_timer() and stop_timer()
  • I have pushed some comment updates to the trac2225_statistics branch. Please review them carefully and check if they are ok.

I've read all. The changes are mainly based on English grammar, I think. These are very polite for other readers. Thanks a lot. I have to learn English more:(.

I've also revised prefixes in the commit messages (trac2225_ -> 2225_). So the git IDs might be changed. Sorry for confusion.

comment:34 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

I have added 3 more documentation update commits. Please review them. The rest of this branch looks fine, so if you are satisfied with the 3 commits, please go ahead and merge.

comment:35 in reply to: ↑ 34 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund san,

The changes are fine for me. Thank you very much.
BTW I found some mistakes. I've pushed at git 2a337c7. Could you review it again?
If OK, I'll merge it and push. Please let me know.

Regards,

comment:36 in reply to: ↑ 35 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

Replying to naokikambe:

Hi Mukund san,

The changes are fine for me. Thank you very much.
BTW I found some mistakes. I've pushed at git 2a337c7. Could you review it again?
If OK, I'll merge it and push. Please let me know.

I have merged a minor FIXME comment about the delta.total_seconds() change. Please review it.

Please go ahead and merge.

comment:37 in reply to: ↑ 36 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund san,

Thank you for updating. Your updating makes sense to me. I've merged the codes. Also I've rebased trac2225_xfrout banch (79a11). It's ready for re-reviewing. Could you still continue to review it? If not, please set a reviewer to UnAssigned. Thanks in advance,

comment:38 follow-up: Changed 7 years ago by muks

Hi Kambe san

See this URL for a unittest that is failing due to the trac2225_statistics branch merge:
http://git.bind10.isc.org/~tester/builder//BIND10-cppcheck/20130125000502-FreeBSD8-amd64-GCC/logs/unittests.out

Please fix it when you get the chance. :)

comment:39 in reply to: ↑ 38 ; follow-up: Changed 7 years ago by naokikambe

Hi Mukund san,

Thank you for suggesting. I've made a branch trac2225_statistics_2(89fd37b) for it. Could you review it?

Thanks,

comment:40 in reply to: ↑ 39 ; follow-up: Changed 7 years ago by muks

Replying to naokikambe:

Hi Mukund san,

Thank you for suggesting. I've made a branch trac2225_statistics_2(89fd37b) for it. Could you review it?

This looks good. Please go ahead and merge. :)

comment:41 in reply to: ↑ 40 Changed 7 years ago by naokikambe

Replying to muks:

This looks good. Please go ahead and merge. :)

I've merged it. Thanks,

comment:42 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

Here is my review of the trac2225_xfrout branch.

  • In xfrin.py.in, Can't the following code be simplified? The replace method calls assume that DATAROOTDIR has the word share in it anyway. BTW, does this original modification have any reason to be in the trac2225_xfrout branch?
    # If B10_FROM_BUILD or B10_FROM_SOURCE is set in the environment, we
    # use data files from a directory relative to that, otherwise we use
    # the ones installed on the system
    PREFIX = "@prefix@"
    DATAROOTDIR = "@datarootdir@"
    if "B10_FROM_BUILD" in os.environ:
        AUTH_SPECFILE_PATH = os.environ["B10_FROM_BUILD"] + "/src/bin/auth"
    else:
        AUTH_SPECFILE_PATH = "@datadir@/@PACKAGE@".replace("${datarootdir}", DATAROOTDIR).replace("${prefix}", PREFIX)
    if "B10_FROM_SOURCE" in os.environ:
        SPECFILE_PATH = os.environ["B10_FROM_SOURCE"] + "/src/bin/xfrin"
    else:
        SPECFILE_PATH = "@datadir@/@PACKAGE@".replace("${datarootdir}", DATAROOTDIR).replace("${prefix}", PREFIX)
    SPECFILE_LOCATION = SPECFILE_PATH + "/xfrin.spec"
    AUTH_SPECFILE_LOCATION = AUTH_SPECFILE_PATH + "/auth.spec"
    

Instead of this, we can use something like:

# If B10_FROM_BUILD or B10_FROM_SOURCE is set in the environment, we
# use data files from a directory relative to that, otherwise we use
# the ones installed on the system
PREFIX = "@prefix@"
SPECFILE_PATH = "@prefix@/share/@PACKAGE@"
AUTH_SPECFILE_PATH = SPECFILE_PATH
if "B10_FROM_BUILD" in os.environ:
    AUTH_SPECFILE_PATH = os.environ["B10_FROM_BUILD"] + "/src/bin/auth"
if "B10_FROM_SOURCE" in os.environ:
    SPECFILE_PATH = os.environ["B10_FROM_SOURCE"] + "/src/bin/xfrin"
SPECFILE_LOCATION = SPECFILE_PATH + "/xfrin.spec"
AUTH_SPECFILE_LOCATION = AUTH_SPECFILE_PATH + "/auth.spec"
  • Is the same change (above) required in xfrout.py.in where B10_FROM_SOURCE doesn't seem to be used?
  • In the code, as an example, socket/unixdomain/senderr is incremented. But the b10-xfrout manpage does not mention socket/unixdomain/.
  • In UnixSockServer.__init__(), what happens now if ThreadingUnixStreamServer.__init__() raises an exception?
    -        ThreadingUnixStreamServer.__init__(self, sock_file, handle_class)
    +        self._counters = Counters(SPECFILE_LOCATION)
    +        try:
    +            ThreadingUnixStreamServer.__init__(self, sock_file, \
    +                                                   handle_class)
    +        except:
    +            self._counters.inc('socket', 'unixdomain', 'openfail')
    +        else:
    +            self._counters.inc('socket', 'unixdomain', 'open')
    

Should it not raise the exception again after updating the counter?
There should be an assertRaises check in the corresponding testcase
too (in test_open()).

  • Here, why is close incremented inside the try block, when it's already shutdown outside the try block?
        def shutdown(self):
            self._write_sock.send(b"shutdown") #terminate the xfrout session thread
            super().shutdown() # call the shutdown() of class socketserver_mixin.NoPollMixIn
            try:
                # count closed unixsockets
                self._counters.inc('socket', 'unixdomain', 'close')
                os.unlink(self._sock_file)
            except Exception as e:
                logger.error(XFROUT_REMOVE_UNIX_SOCKET_FILE_ERROR, self._sock_file, str(e))
    
  • A couple of NotifyOut tests seem to have been removed. Why?
  • Is there anything left to be done for this unittest:
        def test_senderr(self):
            # do inside of above TestXfroutSession
            pass
    
  • I have not tested the lettuce changes as I have issues running lettuce on my box with Python 3.3 currently. I'll do this during the next round of review.
  • I've also pushed some minor patches. Please check them.

comment:43 follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hello Mukund san,

Thank you for the comments, but I've pushed small changes for the following failures. The branch name is trac2225_statistics_3(ffd4a28). Could you review them at first? While you are reviewing, I'll work for your above comments. Thanks,

AssertionError: 0.0 not greater than 0

http://git.bind10.isc.org/~tester/builder//BIND10-cppcheck/20130129171501-FreeBSD8-amd64-GCC/logs/unittests.out
http://git.bind10.isc.org/~tester/builder//BIND10-cppcheck/20130205115001-FreeBSD8-amd64-GCC/logs/unittests.out

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

comment:44 in reply to: ↑ 43 Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

Replying to naokikambe:

Thank you for the comments, but I've pushed small changes for the following failures. The branch name is trac2225_statistics_3(ffd4a28). Could you review them at first? While you are reviewing, I'll work for your above comments. Thanks,

The patch looks good to me. Please go ahead and merge it. :)

comment:45 in reply to: ↑ 42 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund san,

I've already merged trac2225_statistics_3. Thank you for reviewing so quickly.

BTW here is my replying to your comments to trac2225_xfrout.

  • In xfrin.py.in, Can't the following code be simplified? The replace method calls assume that DATAROOTDIR has the word share in it anyway. BTW, does this original modification have any reason to be in the trac2225_xfrout branch?

I think some users may change DATAROOTDIR to the directry other than share when they configures. So I left it. But I've changed the code for simplifying. Could you see this?

35c6e4a  2013-02-06  [2225_xfrout] setting SPECFILE_LOCATION is more simplified

Of course, this change isn't directly needed for current trac2225_xfrout. I needed in the old python statistics library.

  • Is the same change (above) required in xfrout.py.in where B10_FROM_SOURCE doesn't seem to be used?

Of course, it isn't required. But xfrout.spec is surely placed under B10_FROM_SOURCE. If some code other than b10-xfrout calls it from the build or source directly with the environment variable, it should be set properly. I don't now have strong opinion for it.

  • In the code, as an example, socket/unixdomain/senderr is incremented. But the b10-xfrout manpage does not mention socket/unixdomain/.

socket/unixdomain/senderr is mentioned here. Do you mean about how we should describe about it? Or should we change the name to socket/unix/****?

b10-xfrout.xml:

      <varlistentry>
        <term>senderr</term>
        <listitem><simpara>
         UNIX domain sockets send errors
        </simpara></listitem>
      </varlistentry>
  • In UnixSockServer.__init__(), what happens now if ThreadingUnixStreamServer.__init__() raises an exception?

...

Should it not raise the exception again after updating the counter?
There should be an assertRaises check in the corresponding testcase
too (in test_open()).

I changed it to raise an exception again and to test for it. Could see this?

* 67b451a  2013-02-06  [2225_xfrout] raise again an exception after incrementing the counter
  • Here, why is close incremented inside the try block, when it's already shutdown outside the try block?

Yes, that isn't needed. I've moved it out of the try block.

* 6ed2eb9  2013-02-06  [2225_xfrout] counting closed unixdomain sockets doesn't need to be inside the try block
  • A couple of NotifyOut tests seem to have been removed. Why?

Of course, I removed some tests for XfroutCouter class long time ago(git c06c560). But the similar tests are done in other test classes. For example, regarding NotifyOut class, I think the tests for it are done under isc/notify/notify_out/tests as you know.

  • Is there anything left to be done for this unittest:

It isn't needed. I've removed at:

8bfa9b4  2013-02-06  [2225_xfrout] remove an empty unittest
  • I have not tested the lettuce changes as I have issues running lettuce on my box with Python 3.3 currently. I'll do this during the next round of review.

OK, I'll wait for your second round.

  • I've also pushed some minor patches. Please check them.

That is reasonable for me. Thank you very much.

Regards,

comment:46 in reply to: ↑ 45 ; follow-up: Changed 7 years ago by muks

Hi Kambe san

Replying to naokikambe:

  • In the code, as an example, socket/unixdomain/senderr is incremented. But the b10-xfrout manpage does not mention socket/unixdomain/.

socket/unixdomain/senderr is mentioned here. Do you mean about how we
should describe about it? Or should we change the name to
socket/unix/****?

b10-xfrout.xml:

      <varlistentry>
        <term>senderr</term>
        <listitem><simpara>
         UNIX domain sockets send errors
        </simpara></listitem>
      </varlistentry>

Only "senderr" seems to be mentioned in the manpage, but not the
hierarchy up to senderr.

The rest seems ok to me. I'm still not able to run all of the lettuce
tests due to unrelated crashes, but xfrin_notify_handling.feature runs
OK.

Also please look at the failure in #2741.

comment:47 Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

comment:48 in reply to: ↑ 46 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hi Mukund san,

Replying to muks:

Also please look at the failure in #2741.

I've pushed trac2225_statistics_4(c2999ba) for it. I'd like you to review it. Regarding comments related to xfrout, I'll check later.

Regards,

comment:49 in reply to: ↑ 48 ; follow-up: Changed 7 years ago by muks

Replying to naokikambe:

I've pushed trac2225_statistics_4(c2999ba) for it. I'd like you to review it. Regarding comments related to xfrout, I'll check later.

The branch looks ok. Does the attached patch seem reasonable?

comment:50 Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

comment:51 in reply to: ↑ 49 ; follow-up: Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hello Mukund san,

Replying to muks:

The branch looks ok. Does the attached patch seem reasonable?

Yes. I've applied it and merged. Thanks.

Replying to muks:

Only "senderr" seems to be mentioned in the manpage, but not the
hierarchy up to senderr.

I updated descriptions of zones, socket and unixdomain in the spec file, and updated the man page. I redefined them as directories. Could you refer to trac2225_xfrout(aed7be6)?

I'm still not able to run all of the lettuce
tests due to unrelated crashes, but xfrin_notify_handling.feature runs
OK.

Is the issue specific to your environment? :(

Regards,

comment:52 in reply to: ↑ 51 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

Replying to naokikambe:

Replying to muks:

Only "senderr" seems to be mentioned in the manpage, but not the
hierarchy up to senderr.

I updated descriptions of zones, socket and unixdomain in the spec file, and updated the man page. I redefined them as directories. Could you refer to trac2225_xfrout(aed7be6)?

If you render this page to HTML, does it show open under unixdomain or does it show both of them as part of the same list? What I mean is that it has to show the hierarchy. So you can either type them as "socket/unixdomain/open" in the manpage, or use nested lists to show "open" under "unixdomain", and "unixdomain" under "socket". Otherwise, if it is all a flat list of keys, it won't be clear to the reader where to find "open".

To illustrate, it should not be like this:

  • socket
  • unixdomain
  • open
  • openfail
  • close
  • ...

but like this:

  • socket
    • unixdomain
      • open
      • openfail
      • close
      • ...

or if that's not possible, even like this:

  • socket/unixdomain/open
  • socket/unixdomain/openfail
  • socket/unixdomain/close
  • socket/unixdomain/...

Is this reasonable?

I'm still not able to run all of the lettuce
tests due to unrelated crashes, but xfrin_notify_handling.feature runs
OK.

Is the issue specific to your environment? :(

This seems to be. It is unrelated to this branch. But I could run xfrin_notify_handling.feature successfully.

comment:53 in reply to: ↑ 52 Changed 7 years ago by naokikambe

  • Owner changed from naokikambe to muks

Hello Mukund san,

Replying to muks:

  • socket
    • unixdomain
      • open
      • openfail
      • close
      • ...

Thank you for illustrating. I updated the man xml along with a hierarchy like that. Could you refer to the latest branch(ab52e5a)?

This seems to be. It is unrelated to this branch. But I could run xfrin_notify_handling.feature successfully.

I already revised the xfrout code. Probably some other lettuce scenarios test it. I'll try to check again after reviewing.

Regards,

comment:54 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to naokikambe

Hi Kambe san

It looks good to me now. Please go ahead and merge.

comment:55 in reply to: ↑ 54 Changed 7 years ago by naokikambe

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

Hello Mukund san,

I put it through build bot, then it passed. I've merged the branch. I'm closing the ticket.

Thank you for reviewing for a long time.

Note: See TracTickets for help on using tickets.