Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2138 closed enhancement (fixed)

update the Auth module according to the new statistics model

Reported by: y-aharen Owned by: fujiwara
Priority: medium Milestone: Sprint-20120904
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 3.5 Internal?: no

Description (last modified by y-aharen)

update the Auth module according to the new model of #2135.

Implement 'pull' mode into Auth module to send statistics items to Stats module, formatted as described in StatsModule.

Subtickets

Change History (19)

comment:1 Changed 7 years ago by jinmei

We need more details of this ticket so we can give a reasonable
estimate. what exactly is expected to do in this task?

comment:2 Changed 7 years ago by y-aharen

  • Description modified (diff)

comment:3 Changed 7 years ago by shane

  • Milestone changed from New Tasks to StatsRedesign

comment:4 Changed 7 years ago by fujiwara

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

comment:5 Changed 7 years ago by fujiwara

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

The branch is ready for reviewing.

comment:6 Changed 7 years ago by jelte

  • Milestone changed from StatsRedesign to Sprint-20120821

comment:7 follow-up: Changed 7 years ago by y-aharen

For reviewing, please also refer to branch 'stats201209-auth-merge'.

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

Replying to y-aharen:

For reviewing, please also refer to branch 'stats201209-auth-merge'.

I don't understand what this means. Could you elaborate?

Anyway, the following are my review comments on the branch (I've not
looked into the stats201209-auth-merge branch).

general

  • I believe we need a changelog entry for this ticket.
  • It looks like we need to update the BIND 10 guide too.
  • I thought auth will still need to send statistics spontaneously, in case it has a potentially large amount of statistics data, such as in the case of enabling per-zone statistics, and the stats module doesn't try to get it for a while. So I'm not sure if it's a good idea to remove the code related to this behavior completely. What's the plan about it? Are we going to re-add it later?
  • I've seen some long lines and fixed them. Please make sure the style guideline on the expected max number of columns (79) is met.
  • some system tests now fail. Please fix them.

auth_messages.mes

  • AUTH_RECEIVED_GETSTATS: why did you remove the second sentence?
    -a command from the statistics module to send it data. The 'sendstats'
    -command is handled differently to other commands, which is why the debug
    -message associated with it has its own code.
    +a command from the statistics module to send it data.
    
    I really don't know what "handled differently" means, but if the reason is that it's not handled differently any more, it seems to me we can remove the special log at all, rather than renaming it.

command.cc

  • AuthCommand::exec(): please describe the requirement for derived class implementations about the return value in the detailed description part. 'command result data' is not really clear. Assume someone writing a new command class, only referring to this description (not looking into other derived class implementation). We need to provide sufficient information for such developers.
  • why did you remove the second sentence?
    -// Handle the "shutdown" command. An optional parameter "pid" is used to
    -// see if it is really for our instance.
    +// Handle the "shutdown" command.
    
  • execAuthServerCommand: we can return the exec result within the try block and avoid defining the 'value' variable
    {
        LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_RECEIVED_COMMAND).arg(command_id);
        try {
            return (scoped_ptr<AuthCommand>(
                        createAuthCommand(command_id))->exec(server, args));
        } catch (const isc::Exception& ex) {
            LOG_ERROR(auth_logger, AUTH_COMMAND_FAILED).arg(command_id)
                                                       .arg(ex.what());
            return (createAnswer(1, ex.what()));
        }
    }
    
    This may be a matter of taste for such a small function, but I generally prefer avoiding a (especially mutable) variable if we can easily do so because they are often a source of bugs.

statistics.cc

  • redundant spaces in building statistics_string looks awkward. I've fixed them.
  • It doesn't seem to be good that we cannot pass ConstElementPtr to the validator (forgetting that I don't even think it makes sense to try to validate it in the sender side):
        isc::data::ElementPtr statistics_element =
            isc::data::Element::fromJSON(statistics_string);
        // validate the statistics data before send
        if (validator_) {
            if (!validator_(statistics_element)) {
    
    And, in fact, the new version of the code slightly modified the semantics because the validator_ could modify the passed data and we could now return it (previously we passed a copy to the validator_ and always use the build data whatever the validator does to the given data). If it's really the case, we should update the auth side code so we pass a copy; if it's not the case, we should probably revisit the validator interface so we can pass a const pointer (although it would be beyond the scope of this ticket).

command_unittest.cc

  • getStats test: the comment seems to be a naive copy-paste. It's different from what's actually happening.
        // Just check some message has been sent.  Detailed tests specific to
        // statistics are done in its own tests.
    

config_unittest.cc

  • exceptionGuarantee: I suspect you cannot simply remove this test. This looks like testing the exception guarantee in general, not specific to the statistics timer interval. With the removal of this config, we need to find another config parameter to test the guarantee.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to fujiwara

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

Replying to jinmei:

For reviewing, please also refer to branch 'stats201209-auth-merge'.

I don't understand what this means. Could you elaborate?

The branch is only for testing.

general

  • I believe we need a changelog entry for this ticket.

ticket 2136 will add a changelog entry which describes ticket 2136, 2137 and 2138.

  • It looks like we need to update the BIND 10 guide too.

updated

  • I thought auth will still need to send statistics spontaneously, in case it has a potentially large amount of statistics data, such as in the case of enabling per-zone statistics, and the stats module doesn't try to get it for a while. So I'm not sure if it's a good idea to remove the code related to this behavior completely. What's the plan about it? Are we going to re-add it later?

I removed the code because ticket2136 removes Stats side code which receives
spontaneous update of statistics data. We can re-add it later if we need.

  • I've seen some long lines and fixed them. Please make sure the style guideline on the expected max number of columns (79) is met.
  • some system tests now fail. Please fix them.

The reason of system tests failure is that trac2138 changed only auth server.
Changes by trac2136 is required for running system test.

auth_messages.mes

  • AUTH_RECEIVED_GETSTATS: why did you remove the second sentence?
    -a command from the statistics module to send it data. The 'sendstats'
    -command is handled differently to other commands, which is why the debug
    -message associated with it has its own code.
    +a command from the statistics module to send it data.
    
    I really don't know what "handled differently" means, but if the reason is that it's not handled differently any more, it seems to me we can remove the special log at all, rather than renaming it.

"getstats" command uses normal command interface. "sendstats" used another
communication channel.

command.cc

  • AuthCommand::exec(): please describe the requirement for derived class implementations about the return value in the detailed description part. 'command result data' is not really clear. Assume someone writing a new command class, only referring to this description (not looking into other derived class implementation). We need to provide sufficient information for such developers.

changed as "/ \return command result using createAnswer()."

  • why did you remove the second sentence?
    -// Handle the "shutdown" command. An optional parameter "pid" is used to
    -// see if it is really for our instance.
    +// Handle the "shutdown" command.
    

recovered.

  • execAuthServerCommand: we can return the exec result within the try block and avoid defining the 'value' variable
    {
        LOG_DEBUG(auth_logger, DBG_AUTH_OPS, AUTH_RECEIVED_COMMAND).arg(command_id);
        try {
            return (scoped_ptr<AuthCommand>(
                        createAuthCommand(command_id))->exec(server, args));
        } catch (const isc::Exception& ex) {
            LOG_ERROR(auth_logger, AUTH_COMMAND_FAILED).arg(command_id)
                                                       .arg(ex.what());
            return (createAnswer(1, ex.what()));
        }
    }
    
    This may be a matter of taste for such a small function, but I generally prefer avoiding a (especially mutable) variable if we can easily do so because they are often a source of bugs.

used the code.

statistics.cc

  • It doesn't seem to be good that we cannot pass ConstElementPtr to the validator (forgetting that I don't even think it makes sense to try to validate it in the sender side):
        isc::data::ElementPtr statistics_element =
            isc::data::Element::fromJSON(statistics_string);
        // validate the statistics data before send
        if (validator_) {
            if (!validator_(statistics_element)) {
    
    And, in fact, the new version of the code slightly modified the semantics because the validator_ could modify the passed data and we could now return it (previously we passed a copy to the validator_ and always use the build data whatever the validator does to the given data). If it's really the case, we should update the auth side code so we pass a copy; if it's not the case, we should probably revisit the validator interface so we can pass a const pointer (although it would be beyond the scope of this ticket).

changed as "static_cast<isc::data::ConstElementPtr>(statistics_element)"

command_unittest.cc

  • getStats test: the comment seems to be a naive copy-paste. It's different from what's actually happening.
        // Just check some message has been sent.  Detailed tests specific to
        // statistics are done in its own tests.
    

changed as "Just check some message has been received."

config_unittest.cc

  • exceptionGuarantee: I suspect you cannot simply remove this test. This looks like testing the exception guarantee in general, not specific to the statistics timer interval. With the removal of this config, we need to find another config parameter to test the guarantee.

tried to test with server.setListenAddresses() and server.getListenAddress().
It may be incomplete.

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

First off, I assume you meant to pass this ticket back to me based on
the comments and code changes. If it was indeed the case, please make
sure to change the ticket owner. Basically, as long as you own the
ticket the reviewer would assume you're still working on it and
wouldn't resume the review.

See also: http://bind10.isc.org/wiki/CodeReviewProcedure

Replying to fujiwara:

For reviewing, please also refer to branch 'stats201209-auth-merge'.

I don't understand what this means. Could you elaborate?

The branch is only for testing.

This doesn't address my question...my point is that it's not clear
what the reviewer should do by merely being told "please also refer to
XXX". If that branch isn't necessary for reviewing the review target,
it should be clarified; if some part of 'stats201209-auth-merge' needs
to be reviewed within this ticket, it should be clarified; if some
part of 'stats201209-auth-merge' are considered to be needed to check
to understand this branch, it should be clarified, etc.

I didn't see any problem to understand what this branch does for what
purposes without looking into 'stats201209-auth-merge', so this
"please refer to" comment is simply unclear.

general

  • I believe we need a changelog entry for this ticket.

ticket 2136 will add a changelog entry which describes ticket 2136, 2137 and 2138.

Adding the changelog with #2136 is okay, but IMO the changelot text
should be proposed and reviewed here, because the developer and the
reviewer for that particular change should know it best.

  • I've seen some long lines and fixed them. Please make sure the style guideline on the expected max number of columns (79) is met.

Hmm, apparently I forgot to push my local changes. I've merged your
fixes with mine, with some additional editorial changes, and pushed
them.

  • some system tests now fail. Please fix them.

The reason of system tests failure is that trac2138 changed only auth server.
Changes by trac2136 is required for running system test.

In that case please make sure merging to master won't cause system
test regression, either by temporarily disabling some tests or by
carefully organizing merge order.

auth_messages.mes

  • AUTH_RECEIVED_GETSTATS: why did you remove the second sentence?
    -a command from the statistics module to send it data. The 'sendstats'
    -command is handled differently to other commands, which is why the debug
    -message associated with it has its own code.
    +a command from the statistics module to send it data.
    
    I really don't know what "handled differently" means, but if the reason is that it's not handled differently any more, it seems to me we can remove the special log at all, rather than renaming it.

"getstats" command uses normal command interface. "sendstats" used another
communication channel.

This doesn't address my (real) point, which was: if there's now no
reason for having the AUTH_RECEIVED_GETSTATS message code separately
from generic ones, IMO we should rather remove the code.

command.cc

  • AuthCommand::exec(): please describe the requirement for derived class implementations about the return value in the detailed description part. 'command result data' is not really clear. Assume someone writing a new command class, only referring to this description (not looking into other derived class implementation). We need to provide sufficient information for such developers.

changed as "/ \return command result using createAnswer()."

This doesn't seem to be sufficient. I committed my suggested change
to the branch and pushed it at commit f02efa8. Please check it.

statistics.cc

  • It doesn't seem to be good that we cannot pass ConstElementPtr to the validator (forgetting that I don't even think it makes sense to try to validate it in the sender side):

[...]

changed as "static_cast<isc::data::ConstElementPtr>(statistics_element)"

Sorry, I realized I misunderstood the original code. I thought the
validator expects a mutable version of pointer, but the real issue was
getStatistics() was defined to return a mutable pointer. On further
look I noticed it can actually be a const pointer if we change all of
them for the getStatistics variants to const. I made the change
myself at caa70d6.

BTW: conversion from non const to const pointer can be done
implicitly (and always safe), so you don't need the above cast anyway.

command_unittest.cc

  • getStats test: the comment seems to be a naive copy-paste. It's different from what's actually happening.
        // Just check some message has been sent.  Detailed tests specific to
        // statistics are done in its own tests.
    

changed as "Just check some message has been received."

It still doesn't seem to be correct to me because in this test
scenario no message was "sent" or "received". I've committed my own
suggestion at 9463f3e.

config_unittest.cc

  • exceptionGuarantee: I suspect you cannot simply remove this test. This looks like testing the exception guarantee in general, not specific to the statistics timer interval. With the removal of this config, we need to find another config parameter to test the guarantee.

tried to test with server.setListenAddresses() and server.getListenAddress().
It may be incomplete.

I've revised the test and committed it so configureAuthServer() will
actually modify partially the listen_list and rollback to the original
value. The previous one was not really a good test because it didn't
even temporarily change the server state. Note: using the listen_on
may not actually be a good test case for exception guarantee anyway,
because rolling back to previous port is generally difficult and
I suspect we'll eventually have to revisit the behavior (then the test
won't pass at that point). But right now I see no other choice, so I
suggest we keep this test case for the moment.

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

Replying to jinmei:

This doesn't address my question...my point is that it's not clear
what the reviewer should do by merely being told "please also refer to
XXX". If that branch isn't necessary for reviewing the review target,
it should be clarified; if some part of 'stats201209-auth-merge' needs
to be reviewed within this ticket, it should be clarified; if some
part of 'stats201209-auth-merge' are considered to be needed to check
to understand this branch, it should be clarified, etc.

Please ignore 'stats201209-auth-merge' branch in this review.

general

  • I believe we need a changelog entry for this ticket.

ticket 2136 will add a changelog entry which describes ticket 2136, 2137 and 2138.

Adding the changelog with #2136 is okay, but IMO the changelot text
should be proposed and reviewed here, because the developer and the
reviewer for that particular change should know it best.

added changelog entry for tickets 2136, 2137, 2138.

  • some system tests now fail. Please fix them.

The reason of system tests failure is that trac2138 changed only auth server.
Changes by trac2136 is required for running system test.

In that case please make sure merging to master won't cause system
test regression, either by temporarily disabling some tests or by
carefully organizing merge order.

Merging trac2136,2137,2138 should be done in one time, I think.

auth_messages.mes

  • AUTH_RECEIVED_GETSTATS: why did you remove the second sentence?
    -a command from the statistics module to send it data. The 'sendstats'
    -command is handled differently to other commands, which is why the debug
    -message associated with it has its own code.
    +a command from the statistics module to send it data.
    
    I really don't know what "handled differently" means, but if the reason is that it's not handled differently any more, it seems to me we can remove the special log at all, rather than renaming it.

"getstats" command uses normal command interface. "sendstats" used another
communication channel.

This doesn't address my (real) point, which was: if there's now no
reason for having the AUTH_RECEIVED_GETSTATS message code separately
from generic ones, IMO we should rather remove the code.

Removed the message and logging when auth processes "getstats" command.

command.cc

  • AuthCommand::exec(): please describe the requirement for derived class implementations about the return value in the detailed description part. 'command result data' is not really clear. Assume someone writing a new command class, only referring to this description (not looking into other derived class implementation). We need to provide sufficient information for such developers.

changed as "/ \return command result using createAnswer()."

This doesn't seem to be sufficient. I committed my suggested change
to the branch and pushed it at commit f02efa8. Please check it.

Thanks.

statistics.cc

  • It doesn't seem to be good that we cannot pass ConstElementPtr to the validator (forgetting that I don't even think it makes sense to try to validate it in the sender side):

[...]

changed as "static_cast<isc::data::ConstElementPtr>(statistics_element)"

Sorry, I realized I misunderstood the original code. I thought the
validator expects a mutable version of pointer, but the real issue was
getStatistics() was defined to return a mutable pointer. On further
look I noticed it can actually be a const pointer if we change all of
them for the getStatistics variants to const. I made the change
myself at caa70d6.

BTW: conversion from non const to const pointer can be done
implicitly (and always safe), so you don't need the above cast anyway.

Thanks, the change is clear.

command_unittest.cc

  • getStats test: the comment seems to be a naive copy-paste. It's different from what's actually happening.
        // Just check some message has been sent.  Detailed tests specific to
        // statistics are done in its own tests.
    

changed as "Just check some message has been received."

It still doesn't seem to be correct to me because in this test
scenario no message was "sent" or "received". I've committed my own
suggestion at 9463f3e.

config_unittest.cc

  • exceptionGuarantee: I suspect you cannot simply remove this test. This looks like testing the exception guarantee in general, not specific to the statistics timer interval. With the removal of this config, we need to find another config parameter to test the guarantee.

tried to test with server.setListenAddresses() and server.getListenAddress().
It may be incomplete.

I've revised the test and committed it so configureAuthServer() will
actually modify partially the listen_list and rollback to the original
value. The previous one was not really a good test because it didn't
even temporarily change the server state. Note: using the listen_on
may not actually be a good test case for exception guarantee anyway,
because rolling back to previous port is generally difficult and
I suspect we'll eventually have to revisit the behavior (then the test
won't pass at that point). But right now I see no other choice, so I
suggest we keep this test case for the moment.

Thanks.

comment:13 Changed 7 years ago by fujiwara

  • Owner changed from fujiwara to jinmei

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

Replying to fujiwara:

general

  • I believe we need a changelog entry for this ticket.

ticket 2136 will add a changelog entry which describes ticket 2136, 2137 and 2138.

Adding the changelog with #2136 is okay, but IMO the changelot text
should be proposed and reviewed here, because the developer and the
reviewer for that particular change should know it best.

added changelog entry for tickets 2136, 2137, 2138.

Ah, I thought there would be a separate changelog entry for this
ticket. The added entry looks okay in terms of this ticket. The rest
of the text will have to be checked in the corresponding ticket.

I made one final editorial cleanup and pushed it.

This branch looks okay for merge (when others are ready).

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to fujiwara

comment:16 Changed 7 years ago by naokikambe

My merging trac2136 was done. After merging trac2138, please update ChangeLog and uncomment the skipped system tests related to the changes.

Thanks,

comment:17 in reply to: ↑ 14 Changed 7 years ago by fujiwara

Replying to jinmei:

I made one final editorial cleanup and pushed it.

Thanks. I added some minor fixes (remained SENDSTATS).

This branch looks okay for merge (when others are ready).

After merging trac2136, tests/systest passed.
merging done. closing. Thanks very much.

comment:18 Changed 7 years ago by fujiwara

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

comment:19 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 3.5

Please don't forget deleting the branch.

Note: See TracTickets for help on using tickets.