Opened 7 years ago

Closed 7 years ago

#2222 closed enhancement (fixed)

Implement counters into Xfrout (2/3)

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

Description (last modified by naokikambe)

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

ModuleItemin BIND 9 Statistics XML (tree representation under isc/bind/statistics)Description from BIND 9 ManualImplemented in BIND 10?
Xfrout
xfrout.ixfr_running(shown in 'rndc status': xfers running)Number of IXFRs in progressno
xfrout.axfr_running(shown in 'rndc status': xfers running)Number of AXFRs in progressno

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. Most of fundamental implementation has been done in #2158, so this ticket would not be so difficult.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by naokikambe

  • Description modified (diff)

comment:2 Changed 7 years ago by naokikambe

A proposed ChangeLog entry:

nnn.    [func]          naokikambe
        New statistics items added into Xfrout : ixfr_running add axfr_running.
        Their values can be obtained by invoking "Stats show Xfrout" via bindctl
        when Xfrout is running.
        (Trac #2222, git TBD)

comment:3 Changed 7 years ago by naokikambe

  • Status changed from new to accepted

comment:4 Changed 7 years ago by naokikambe

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

The ticket can be reviewed.

comment:5 Changed 7 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20120925

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

It's difficult to extract the diff to be reviewed. Can you make it
in the form of a diff file or create a separate branch for review
where we can easily identify the commits to be reviewed?

(Or, if someone else can identify the part of code to be reviewed,
please just go ahead).

comment:7 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to naokikambe

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

  • Owner changed from naokikambe to UnAssigned

Replying to jinmei:

It's difficult to extract the diff to be reviewed. Can you make it
in the form of a diff file or create a separate branch for review
where we can easily identify the commits to be reviewed?

I've rebased the branch on the newest master and pushed it. Please recreate the local branch. Is it easier for you to review?

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

I'm not sure if I lack python experience, and I'm not at my best today
generally. But I find a lot of the code there very hard to read or completely
unreadable.

These are hard to read. Would it be possible to simplify it somehow? Or at
least comment what is happening and how?

            incrementer = \
                dict(self.xfrout_counter.get_counters_for_xfroutsession(), \
                         **self.xfrout_counter.get_counters_for_notifyout())\
                         ['counter_%s' % counter_name]

As well as these (I'd suggest using full-blown if-else here):

            self._statistics_data[n] \
            if n.find('ixfr_') == 0 or n.find('axfr_') == 0 \
            else self._statistics_data['zones'][TEST_ZONE_NAME_STR][n]

And this is just too much for me, I have no clue what this code does. The
comment about argument does not help a bit, I have no idea what argument it
talks about:

        # Set counter handlers for counting Xfr requests or
        # incrementing or decrementing Xfr running. An argument
        # is required for zone name in counting Xfr requests.
        for (k, v) in counters.items():
            if k.find('counter_') == 0 or k.find('inc_') == 0 \
                    or k.find('dec_') == 0:
                setattr(self, "_%s" % k, v)

Also, some tests seem to have quite a lot of levarage. I saw similar one 3
times I think. Could the Greater be equal to 1? Because it should increase the
counter by exactly one transfer:

         self.assertEqual(self.get_counter('xfrrej'), 0)
         self.check_transfer_acl(acl_setter)
         self.assertGreater(self.get_counter('xfrrej'), 0)

And, does this test check anything, or is it completely useless? It loosely
translates to „the counter may or may not have been incremented. Whatever the
server likes.“

    And when I query statistics axfr_running of bind10 module Xfrout with cmdctl port 47804
    # xfering might not be started yet.
    Then the statistics counter axfr_running should be between 0 and 1

And, also, it is mostly aesthetics, but the xxcrementer used at few places
sounds very much like excrementer, and while I don't think anyone would ever use
that word in English, it quite undoubtedly means a person sitting
on a toilet at the moment. I'm not sure how conservative we are in naming
things, but this looks like too much amusing name compared with the rest of the
code O:-).

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

  • Owner changed from naokikambe to vorner

Hello,

Replying to vorner:

I'm not sure if I lack python experience, and I'm not at my best today
generally. But I find a lot of the code there very hard to read or completely
unreadable.

Sorry for the unreadable codes. Please point out if there are other ones. I would like to revise as much as possible.

These are hard to read. Would it be possible to simplify it somehow? Or at
least comment what is happening and how?

            incrementer = \
                dict(self.xfrout_counter.get_counters_for_xfroutsession(), \
                         **self.xfrout_counter.get_counters_for_notifyout())\
                         ['counter_%s' % counter_name]

I revised that code. Please see:

396bb78 [2222] simplified complicated assignment of `incrementer`

As well as these (I'd suggest using full-blown if-else here):

            self._statistics_data[n] \
            if n.find('ixfr_') == 0 or n.find('axfr_') == 0 \
            else self._statistics_data['zones'][TEST_ZONE_NAME_STR][n]

I used the full-blown if-else style. Please see:

2fee008 [2222] expanded the lambda statement to the normal method definition

And this is just too much for me, I have no clue what this code does. The
comment about argument does not help a bit, I have no idea what argument it
talks about:

        # Set counter handlers for counting Xfr requests or
        # incrementing or decrementing Xfr running. An argument
        # is required for zone name in counting Xfr requests.
        for (k, v) in counters.items():
            if k.find('counter_') == 0 or k.find('inc_') == 0 \
                    or k.find('dec_') == 0:
                setattr(self, "_%s" % k, v)

I revised the comment. What about this?

7044579 [2222] corrected the comment according to what the code does

Also, some tests seem to have quite a lot of levarage. I saw similar one 3
times I think. Could the Greater be equal to 1? Because it should increase the
counter by exactly one transfer:

         self.assertEqual(self.get_counter('xfrrej'), 0)
         self.check_transfer_acl(acl_setter)
         self.assertGreater(self.get_counter('xfrrej'), 0)

I revised the code to check the counter incremented to one. Please see:

1792477 [2222] changed to check the counter exactly incremented to one

And, does this test check anything, or is it completely useless? It loosely
translates to „the counter may or may not have been incremented. Whatever the
server likes.“

    And when I query statistics axfr_running of bind10 module Xfrout with cmdctl port 47804
    # xfering might not be started yet.
    Then the statistics counter axfr_running should be between 0 and 1

I'm not sure it is completely useless, but I revised the comment:

668cdb3 [2222] updated the comment to be detailed

And, also, it is mostly aesthetics, but the xxcrementer used at few places
sounds very much like excrementer, and while I don't think anyone would ever use
that word in English,

Thank you for the suggestion. I don't really know that meaning.:-)
I changed the method name. Please see:

48df8a1 [2222] changed the misleading method name

Regards,

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

  • Owner changed from vorner to naokikambe

Hello

Replying to naokikambe:

And, does this test check anything, or is it completely useless? It loosely
translates to „the counter may or may not have been incremented. Whatever the
server likes.“

    And when I query statistics axfr_running of bind10 module Xfrout with cmdctl port 47804
    # xfering might not be started yet.
    Then the statistics counter axfr_running should be between 0 and 1

I'm not sure it is completely useless, but I revised the comment:

668cdb3 [2222] updated the comment to be detailed

Actually, I was not worried about the comment of the test, but about the test
itself. What is the purpose of the test? Is there some scenario in which it
could fail? I mean ‒ the test passes if it increments and it passes if it does
not increment.

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

  • Owner changed from naokikambe to vorner

Hello,

Replying to vorner:

Actually, I was not worried about the comment of the test, but about the test
itself. What is the purpose of the test? Is there some scenario in which it
could fail? I mean ‒ the test passes if it increments and it passes if it does
not increment.

I think that test makes sense in a very rare situation.

axfr_running can be 1 while it is between XFROUT_XFR_TRANSFER_STARTED and XFROUT_XFR_TRANSFER_DONE. In my environment that was:

2012-09-25 09:55:08.114 INFO  [b10-xfrout.xfrout] XFROUT_XFR_TRANSFER_STARTED AXFR client [::1]:56174: transfer of zone example.org/IN has started
 ...
2012-09-25 09:55:08.117 INFO  [b10-xfrout.xfrout] XFROUT_XFR_TRANSFER_DONE AXFR client [::1]:56174: transfer of example.org/IN complete

The time difference was only 0.03 seconds.

OTOH in this situation the stats module collects statistics from xfrout at least every second. In the above case axfr_running cannot be 1 because transferring is already done when querying statistics. However this test would make sense if transferring takes more than one second. For that we should use so large size of zone. Would we like to do that in lettuce? Otherwise I think we shouldn't do that because this test doesn't make sense in most situation.

So may I remove this test from xfrin_notify_handling.feature?

Regards,

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

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

Hello

Replying to naokikambe:

Replying to vorner:

OTOH in this situation the stats module collects statistics from xfrout at least every second. In the above case axfr_running cannot be 1 because transferring is already done when querying statistics. However this test would make sense if transferring takes more than one second. For that we should use so large size of zone. Would we like to do that in lettuce? Otherwise I think we shouldn't do that because this test doesn't make sense in most situation.

I think there may be better and more reliable ways for this kind of test. What
I'd do is creating a fake client. It would start the transfer, but it would
pause for a while before reading data. The zone would have to be large enough
to fill the TCP buffer, but not that huge to take over a second by itself.

But that's obviously too large for this ticket. Should I create a new one for
it, or do you want to create it yourself?

So may I remove this test from xfrin_notify_handling.feature?

Yes, please remove it.

After the removal, it seems good enough for merge.

Thank you

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

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 2.15 to 13.40

Hello,

Replying to vorner:

I think there may be better and more reliable ways for this kind of test. What
I'd do is creating a fake client. It would start the transfer, but it would
pause for a while before reading data. The zone would have to be large enough
to fill the TCP buffer, but not that huge to take over a second by itself.

But that's obviously too large for this ticket. Should I create a new one for
it, or do you want to create it yourself?

I think the above test is too heavy for a lettuce test. Not only regarding testing axfr_running, I think we should consider how we test a situation transferring a huge zone. I'd like to give you a way to create a new ticket for that. Please go head.

Yes, please remove it.

After the removal, it seems good enough for merge.

Thank you very much. I've merged and am closing.
(I'll add 11.25 to the total hours.)

Regards,

Note: See TracTickets for help on using tickets.