Opened 10 years ago

Closed 10 years ago

#43 closed task (fixed)

review: src/lib/config

Reported by: jreed Owned by: jreed
Priority: very high Milestone: 02. Running, functional authoritative-only server
Component: Unclassified 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?:

Description

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. If there is already code review in process, please refer to the other ticket number.

Subtickets

Change History (8)

comment:1 Changed 10 years ago by jelte

Make that revision 1045 in trunk/

These are the supporting classes for configuration;

There is a python and a cpp version;

for the python version the relevant files are src/lib/config/python/isc/config/*.py, *except* cfgmgr.py (which is reviewed in ticket 38)

the cpp equivalent version is in src/lib/config/cpp/

ccsession defines ModuleSession? and uimodulesession, which keep a connection to the configuration manager daemon, and handle updates. They are subclasses of ConfigData? (and MultiConfigData? respectively. Those classes hold the module definitions (ModuleSpec?) and actual configuration data (in python stored as native types, in cpp as ElementPtr?, reviewed in ticket 20)

comment:2 Changed 10 years ago by shane

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

Michael has some familiarity with the configuration stuff, so I'm asking him to review this code.

comment:3 Changed 10 years ago by mgraff

Here is my review of the C++ code, with python review coming shortly.

I believe all of these change suggestions can wait for y2.

In session.cc ModuleCCSession::init() there is a sleep(1) remaining. Has this been fixed, or the problem report added to the todo list for the boss process?

Should errors be fatal, rather than just logging to stderr?

There are no tests for addRemoteConfig, removeRemoteConfig, getRemoteConfigValue, or updateRemoteConfig.

Some "expect failure" tests should be added to catch the red parts of the test report: http://bind10.isc.org/~tester/LATEST_UNITTEST_COVERAGE/lib/config/module_spec.cc.gcov.html

It should be pretty easy to catch these.

comment:4 Changed 10 years ago by mgraff

  • Owner changed from mgraff to jelte

Python review:

The tests failed because they referred to 'unittests' directory rather than 'tests' -- I fixed this on trunk, but some tests then failed to run.

Do we really want get_session()? That is, should the client ever need to directly access the session channel that is in use?

get_socket() should be documented that the only reason it is there is to allow select() and the like to be called on it. It should NEVER be used to issue other I/O.

comment:5 Changed 10 years ago by jelte

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

Added tests for those addRemoteConfig etc in the cpp version, updated get_socket documentation, removed get_session().

Moved the rest to the TODO for after Y1 release.

Assuming this is all ok, i think this code is ready for merge.

comment:6 Changed 10 years ago by jreed

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:7 Changed 10 years ago by jreed

  • Owner changed from jelte to jreed
  • Status changed from reopened to assigned

comment:8 Changed 10 years ago by jreed

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

I merged to the reviewed branch.

Note: See TracTickets for help on using tickets.