Opened 9 years ago

Closed 9 years ago

#1060 closed task (complete)

introduce the abstract DataSourceClient and ZoneHandle classes

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20110802
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a first step of data source refactoring.

The amount of change will be a bit big but it should be essentially a
trivial task.

Subtickets

Change History (14)

comment:1 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 5

comment:2 Changed 9 years ago by stephen

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

comment:3 Changed 9 years ago by jinmei

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

comment:4 Changed 9 years ago by jinmei

I've committed my work so far and will release the ownership for now.
Anyone willing to take over this ticket: please afeel free to continue or ignore
it.

comment:5 Changed 9 years ago by jinmei

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

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner
  • Status changed from assigned to accepted

comment:7 follow-up: Changed 9 years ago by vorner

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

I think it should be ready for review. The in-memory data source is inherited from it, with some renaming, etc.

However, the DataSourceClient? class is not complete, I didn't add any of the functions allowing modifications or iterating trough the whole zone. I think these can be added when they are needed (either when they are used or when some backend would like to implement it), as I wouldn't guess the interface right anyway.

I hope I understand the general direction of refactoring correctly. If not, please point it out.

comment:8 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

Replying to vorner:

I think it should be ready for review. The in-memory data source is inherited from it, with some renaming, etc.

However, the DataSourceClient? class is not complete, I didn't add any of the functions allowing modifications or iterating trough the whole zone. I think these can be added when they are needed (either when they are used or when some backend would like to implement it), as I wouldn't guess the interface right anyway.

I hope I understand the general direction of refactoring correctly. If not, please point it out.

Thanks for taking this on. Your changes generally look good.

I've made some additional documentation fixes to the branch directly.
In addition to trivial fixes like correcting typo, it contains:

  • more detailed and more complete documentation for the DataSourceClient? class
  • update the text around ZoneFinder? - no that it's an accessor rather than a zone itself, some of the original text like "NS record in ZoneFinder?" do not really make sense. I've updated those.

I suspect there are other cases where we'd need to make this type of
changes, but since this is a blocking ticket and it's unlikely for me
to find time for comprehensive re-read and fix, I suggest we move on
and fix them in subsequent tickets as we find them.

A few more points:

  • maybe we shoud rename variables like 'zone' to 'zone_finder' (or if it's too long something like 'zfinder')
  • MemoryZoneFinder? should be named InMemoryZoneFinder? (for consistency)?
  • I think InMemoryClient::addZone() should better take MemoryZoneFinder? (because it's specific to this derived class), but I found it would require changes to auth server tests and may make this branch unnecessarily big. So I'm okay to defer it to a separate ticket.

comment:10 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

A few more points:

  • maybe we shoud rename variables like 'zone' to 'zone_finder' (or if it's too long something like 'zfinder')

I tried to find them, but I must admit, it is hard without reading through all our code, which would take a long time. Could we just keep fixing the names as we go or live with this?

Renamed.

  • I think InMemoryClient::addZone() should better take MemoryZoneFinder? (because it's specific to this derived class), but I found it would require changes to auth server tests and may make this branch unnecessarily big. So I'm okay to defer it to a separate ticket.

I agree with the ticket, let's create one when this is merged.

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

Replying to vorner:

A few more points:

  • maybe we shoud rename variables like 'zone' to 'zone_finder' (or if it's too long something like 'zfinder')

I tried to find them, but I must admit, it is hard without reading through all our code, which would take a long time. Could we just keep fixing the names as we go or live with this?

Okay.

I made a couple of trivial/minor fixes, with which I think this branch
can be merged.

One other thing I forgot to mention in the previous comment:

    // TODO: Do we still need the PImpl if nobody should manipulate this class
    // directly any more (it should be handled through DataSourceClient)?

I guess pimpl will become less important for this class (but note that
it's explicitly used in auth at the moment). If avoiding pimpl
improves readability, etc, I have no problem with making that change.

comment:13 Changed 9 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:14 Changed 9 years ago by vorner

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

The pimpl ‒ it can freely wait when auth starts using the classes in generic way and when someone has the time. I just wanted to note it might be changed without expecting much problems.

Merged, closing.

Note: See TracTickets for help on using tickets.