Opened 7 years ago

Closed 7 years ago

#2203 closed task (complete)

separate CC Session from auth DataSourceConfigurator

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20121009
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: background zone loading
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 6.5 Internal?: no

Description

A subtask of #2201.

Not directly related to the feature goal, but (I believe) this
refactor will help later development.

What to do: instead of passing Session to the configurator ('s init()),
define a separate command handler somewhere in auth, and do
addRemoteConfig() with it. The command handler will call
DataSourceConfigurator::reconfigure().

also consider whether we can avoid using the configurator class as
a singleton (i.e. a static object). singleton is often the source
of evils and often makes test harder due to its static nature.

Subtickets

Change History (16)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120904
  • Priority changed from medium to low

comment:2 Changed 7 years ago by jelte

Adding this to current sprint at a slightly lower priority, in case #2109, #2110, #2218 and #2219 are all blocked on #2108

(if not, or if #2108 is nearly done, please pick one of those tickets)

comment:3 Changed 7 years ago by jinmei

  • Priority changed from low to medium

comment:4 Changed 7 years ago by jelte

  • Milestone Sprint-20120918 deleted

comment:5 Changed 7 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20121009

comment:7 Changed 7 years ago by jinmei

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

comment:8 Changed 7 years ago by jinmei

trac2203 is ready for review.

After a series of refactoring, DataSourceConfigurator::reconfigure()
resulted in a totally stateless, stand-alone method. So I decided to
make it a free function and remove the DataSourceConfigurator class
completely. I believe it'll be a more friendly interface when we do
the successor ticket, #2204 (where this class/function will even be
independent from AuthSrv object).

Essentially, what this branch does is to move the static session and
server from the DataSourceConfigurator (or the new free function),
and move the content of the reconfigure() method to the new
configureDataSourceGeneric() function. configuration callback is
now handled within the main() function. Other changes are basically
straightforward interface adjustment except for "initialization" and
"cleanup" test. Since the configurator itself is now stateless, it
doesn't have the concept of init or cleanup. So these tests are
mostly cleaned up.

One possibly controversial change is commit 4fa694d. I changed the
callback type for remote modules from bare function pointer to
boost::function so we can easily pass extra parameter or using it with
a pair of ojbect and its method. We could complete this branch
without this extension, but this change will make the other part of
this branch simpler and (IMO) more readable. And, since
boost::function is basically upper compatible with function pointers,
we don't have to change the rest of the code.

Finally, I addressed the issue of #2291 at commit c91bffd. I believe,
with the extended callback, it's now less error prone than I
originally tried in #2219. But if this change looks too irrelevant or
is still too dirty, I'm fine with excluding this particular piggy
back.

I don't see the need for a separate changelog for this ticket.

(Adding this, originally forgotten) if the overall changes make sense,
I'd also propose changing the file name datasrc_configurator.{h,cc}
to datasrc_config.{h,cc} because "configurator" sounds like an object
but the main logic is now implemented as a function. (But I didn't
make this change to keep the changes small)

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

comment:9 Changed 7 years ago by jinmei

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

comment:10 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:11 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

In general the changes look OK, and I'm fine with both the piggy-back change and the boost::function addition.

A problem though (kind of unrelated to this ticket itself, as it looks like this was already there); the rollback code upon '...' exception (datasrc_configurator.h:95) should not rethrow; as per current specification of addRemoteConfig the handler should never throw (the exception will fall through and auth will abort).

Either we need to add another catch-all (+log error) in the code that calls the handler, or we need to log here, roll back, and not re-throw. The original reasoning of not catching in the caller of the handler was that that caller has, apart from logging an error, no real way of recovering from whatever caused this exception (and hence the handler should already be handling recovery and hence the exception should not even escape). We can discuss this, and we can defer it to another ticket, but we should certainly address it (as right now rolling back at this point is quite useless).

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

Thanks for the review.

Replying to jelte:

In general the changes look OK, and I'm fine with both the piggy-back change and the boost::function addition.

Okay, good.

A problem though (kind of unrelated to this ticket itself, as it looks like this was already there); the rollback code upon '...' exception (datasrc_configurator.h:95) should not rethrow; as per current specification of addRemoteConfig the handler should never throw (the exception will fall through and auth will abort).

Either we need to add another catch-all (+log error) in the code that calls the handler, or we need to log here, roll back, and not re-throw. The original reasoning of not catching in the caller of the handler was that that caller has, apart from logging an error, no real way of recovering from whatever caused this exception (and hence the handler should already be handling recovery and hence the exception should not even escape). We can discuss this, and we can defer it to another ticket, but we should certainly address it (as right now rolling back at this point is quite useless).

We certainly need to discuss the topic of error handling (and
recovery) in configuration, both for the "local" and remote cases.
For example, as we discussed before, currently different modules can
easily have inconsistent configuration when they share a single
configuration.

But, for this specific code I think we can defer this discussion. In
#2211 the remote config handler will be a simple message forwarder to
the separate "configurator thread", so configureDataSourceGeneric()
won't even be a part of the handler. We'll probably still need to
think about how to deal with exceptions from the data source level in
terms of maintaining the thread with the possibility of seeing
exceptions, but that will also be a topic of later tasks (probably
#2210 and #2212). For now, I'll just add a note in #2211 that the new
handler should be exception free per API contract.

Now, assuming it's okay not to touch this point, I made one final set
of changes as mentioned in the previous comment: rename
"datasrc_configurator" to "datasrc_config". It should be trivial,
but could you make sanity checks?

comment:13 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:14 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

ok

that last change looks fine as well, go ahead and merge

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

Replying to jelte:

ok

that last change looks fine as well, go ahead and merge

Thanks, merge done, closing.

comment:16 Changed 7 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 6.5
Note: See TracTickets for help on using tickets.