Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#38 closed task (fixed)

review: src/bin/cfgmgr

Reported by: jreed Owned by: jelte
Priority: very high Milestone: 04. 2nd Incremental Release: Early Adopters
Component: configuration Version:
Keywords: review Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:


Review what is in trunk as of revision 738 following code review procedures. If this is your code, please add notes here as appropriate. I initially assigned this to the code owner, but this ticket will be assigned to a reviewer.


Attachments (1)

bind10_cfgmgr_refactor.patch (13.0 KB) - added by jelte 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by jelte

Make that revision 1044 in trunk.

This is the configuration manager daemon that stores config settings and notifies modules of config changes.

Files can be found in src/bin/cfgmgr/ and the associated src/lib/config/python/isc/config/

A bit of description on the whole config system can be found in src/lib/config/documentation.txt

The actual ConfigData? classes etc should be reviewd in ticket 43

comment:2 Changed 9 years ago by shane

  • Owner changed from jelte to shane
  • Status changed from new to accepted

comment:3 Changed 9 years ago by shane

  • Owner changed from shane to mgraff
  • Status changed from accepted to assigned

Michael you actually are probably the best person to look at this. If you have no time, then please push back, okay?

comment:4 Changed 9 years ago by mgraff

  • Owner changed from mgraff to shane

The code coverage for tests is very good for these. The remaining few seem to be parts that need to be implemented later, with perhaps one exception in ccsession check_command(). That might need a test, it looks like it's code that was added later perhaps.

I'd like to see tickets opened for the remaining features (or one long ticket for them, or some other list have what is left to do) but I'm ok with the "to do later" nature of the unimplemented parts for now.

Some quick observations: and both have methods that check the type of something using a long if/else statement. This should probably be factored out into a common method (perhaps in the cc code itself) that does something like this, or at least into one common function in this code.

In general I dislike the nested if method for argument checking such as is done in cfgmgr/_handle_get_config(). I find it much easier to read when written as:

if cmd != None:

return isc.config.ccsession.create_answer(0, self.config_data)

as a series of tests, then the functional part of the code once all those have passed. The huge nested if method just means we are going to end up indenting off the right side of the page if another item were to be added. I know it's a comsci sort of thing to have one entry and one exit into a method, but in general I make an exception to that rule for this sort of cleanup.

I know we said 80 columns wasn't a limit, but perhaps some of this can be cleaned up by removing the need for as many prefixes? Can Python import the isc.config namespace into this file, so you can use ccsession.create_command() rather than the long one? That would help a lot I suspect to make things closer to the somewhat preferred 80 column range.

Some methods are rather long. For instance, _handle_set_config() is around 60 lines long, which means scrolling to look at in many cases. Perhaps some of that can be factored out into a few smaller helper methods? This is only one example of this, there are several others that are starting to get a bit too long.

All in all, the code seems good quality, and the test coverage makes me smile.

I'm not certain who gets tagged with this next, but I'll pass it to Shane to pass it along.

comment:5 Changed 9 years ago by mgraff

I forgot to include the C++ review here. Ooops.

The comments need to be changed in the header and perhaps .cc files. It is extremely difficult to read this:

Returns the local (...)
void someFunciton() { ... }
comment here
void functionHere();
comment here

Put spaces to make it easier to match comments with the item they apply to by quick glance.

Nice test coverage here too.

comment:6 Changed 9 years ago by shane

  • Owner changed from shane to jelte

comment:7 Changed 9 years ago by shane

  • Milestone changed from 02. Running, functional authoritative-only server to 04. 2nd Incremental Release

Jelte, looks like maybe a half day of work to address these concerns. Is there time before the next incremental release?

Changed 9 years ago by jelte

comment:8 Changed 9 years ago by jelte

  • Owner changed from jelte to mgraff
  • Status changed from assigned to reviewing

I committed those empty lines in the .h file directly to trunk.

Attached a patch that refactors, please take a look at it to see if it adresses all the concerns you mentioned.

comment:9 Changed 9 years ago by mgraff

  • Owner changed from mgraff to jelte

I like it. Sounds like this ticket is good to close once merged in, I hope :)

comment:10 Changed 9 years ago by jelte

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

Ok, grazie. Committed in r1928, closing ticket.

comment:11 Changed 9 years ago by shane

  • Component changed from Unclassified to configuration
Note: See TracTickets for help on using tickets.