Opened 9 years ago

Closed 9 years ago

#418 closed task (fixed)

new query logic for in memory data source (1.5)

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month milestone
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a next small subtask of the query logic for in memory data source.

I'm going to make the zone class inheritable so that we can test the query logic with a mock zone class, without waiting for the rest of the actual in memory data source. The idea is to extend this notion to other types of data source eventually, but the plan should be (re)assessed at that point.

Subtickets

Change History (8)

comment:1 Changed 9 years ago by jinmei

branches/trac418 is ready for review. Changeset is r3670.

It should be quite trivial. The diff may not look short, but most of the change is straightforward refactoring of introducing an abstract base zone class.

The intent of the newly introduce find() method is documented.

And ideas for future changes are also documented.

With this change, we'll be able to change the Query::process() method of trac #415 as follows:

    if (result.zone->find(qname_, qtype_).code == AbstractZone::NXDOMAIN) {
        response_.setRcode(Rcode::NXDOMAIN());
    }

(instead of hardcode) and will be able to test other cases with a mock class zone class (e.g. one that returns an A RRset for qname="www.example.com" and qtype="A").

comment:2 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing
  • Type changed from defect to task

comment:3 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:4 follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Code changes themselves look ok, one thing:
Should the bare pointer in ZoneTable::FindResult? not be a ConstZonePtr?? (also have a comment on that name, but see below)

I do wonder about the general direction this is heading though; for instance this change implies a new mandatory subclass for each datasource. One or two of those may not be a problem, but it feels like a slippery slope.

If the Zone class is only meant to be a specific subclass from AbstractZone? for the in-memory datasource, I would suggest we rename it, to better reflect that.

If it is possible that we can provide a default class for 'general' datasources that don't need specific features, that's what Zone should be (if not, I think we should consider renaming AbstractZone? to Zone actually, to make calling code more intuitive; Zone::NXDOMAIN vs AbstractZone::NXDOMAIN); and then recheck the shared_ptr typedefs, their names don't match what they encapsulate right now.

Oh and a tiny comment on a comment; I'd say that the FindResult? struct/class is all state, not no state (comment at the definition of the first one zonetable.h line 72) :)

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

  • Owner changed from jinmei to jelte

Replying to jelte:

Should the bare pointer in ZoneTable::FindResult not be a ConstZonePtr?

It's intentional and documented. See the "notes to developer" part of the ZoneTable class description.

I personally think we tend to overuse shared pointers even when the ownership of the objects don't have to be (or are not even supposed to be) shared. I'm afraid it make us less careful about the ownership relationships among classes in design. On the other hand, using shared pointers make code safer in general, and in that sense there's nothing wrong with them except their slightly higher overhead (and I don't worry about the overhead at the moment). So, this is actually an open point. I don't mind changing it to shared pointers if you like that approach.

I do wonder about the general direction this is heading though; for instance this change implies a new mandatory subclass for each datasource. One or two of those may not be a problem, but it feels like a slippery slope.

I don't yet have a clear image about how the zone class would be used when everything is integrated, but I can think of two possibilities:

  • if the underlying database such as some variant of SQL doesn't have an explicit representation of zones (as part of public interface), we can probably use a "default" zone class that simply encapsulates the corresponding data source and calls its find() (or whatever we finally come up with) method
  • some data source may want to specialize it by inheritance as an optimization. for example, in the current schema design of the sqlite3 data source, its zone (derived) class would contain the information of the "zone ID".

I'm not sure if this answers your comment, though...(whether or not it makes sense to you) does it?

If the Zone class is only meant to be a specific subclass from AbstractZone? for the in-memory datasource, I would suggest we rename it, to better reflect that.

If it is possible that we can provide a default class for 'general' datasources that don't need specific features, that's what Zone should be (if not, I think we should consider renaming AbstractZone? to Zone actually, to make calling code more intuitive; Zone::NXDOMAIN vs AbstractZone::NXDOMAIN); and then recheck the shared_ptr typedefs, their names don't match what they encapsulate right now.

Yes, I know the inconsistency between the shared pointer typedef and the class name, and I was aware it's awkward to have the AbstractZone:: prefix in application code. On the other hand, clearly naming an abstract base class has some advantage, e.g., it can emphasize the point that it's expected to define interfaces only and all methods should be pure virtual. So, the current implementation is a compromise on the tradeoff.

I'm okay with renaming AbstractZone? to Zone in preferring consistency, though. (And in any case the current Zone class will be renamed to something like InMemoryZone? anyway). Taking into account the background, would you like to make that change?

Oh and a tiny comment on a comment; I'd say that the FindResult? struct/class is all state, not no state (comment at the definition of the first one zonetable.h line 72) :)

By "no internal state" I meant once constructed it's never changed. But as you pointed out "no state" would not really be accurate for that meaning. How about this?

    /// This is a simple value class whose internal state never changes, so for
    /// convenience we allow the applications to refer to the members

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

Replying to jelte:

Should the bare pointer in ZoneTable::FindResult not be a ConstZonePtr?

It's intentional and documented. See the "notes to developer" part of the ZoneTable class description.

I personally think we tend to overuse shared pointers even when the ownership of the objects don't have to be (or are not even supposed to be) shared. I'm afraid it make us less careful about the ownership relationships among classes in design. On the other hand, using shared pointers make code safer in general, and in that sense there's nothing wrong with them except their slightly higher overhead (and I don't worry about the overhead at the moment). So, this is actually an open point. I don't mind changing it to shared pointers if you like that approach.

Ah, well, I don't necessarily have a problem with not using a shared ptr. The reason it caught my attention is that the other FindResult? does have one :) But it's described, so I'm ok with it (i did miss that part of the notes on my first read somehow).

I don't yet have a clear image about how the zone class would be used when everything is integrated, but I can think of two possibilities:

  • if the underlying database such as some variant of SQL doesn't have an explicit representation of zones (as part of public interface), we can probably use a "default" zone class that simply encapsulates the corresponding data source and calls its find() (or whatever we finally come up with) method
  • some data source may want to specialize it by inheritance as an optimization. for example, in the current schema design of the sqlite3 data source, its zone (derived) class would contain the information of the "zone ID".

I'm not sure if this answers your comment, though...(whether or not it makes sense to you) does it?

Yes, that is what I had in mind too; if we can do this, it both provides the power for specific datasources that need it, and comparative ease of creation for those that don't.

Yes, I know the inconsistency between the shared pointer typedef and the class name, and I was aware it's awkward to have the AbstractZone:: prefix in application code. On the other hand, clearly naming an abstract base class has some advantage, e.g., it can emphasize the point that it's expected to define interfaces only and all methods should be pure virtual. So, the current implementation is a compromise on the tradeoff.

I'm okay with renaming AbstractZone? to Zone in preferring consistency, though. (And in any case the current Zone class will be renamed to something like InMemoryZone? anyway). Taking into account the background, would you like to make that change?

I have a slight preference for that at this point, so yes please :)

By "no internal state" I meant once constructed it's never changed. But as you pointed out "no state" would not really be accurate for that meaning. How about this?

    /// This is a simple value class whose internal state never changes, so for
    /// convenience we allow the applications to refer to the members

awesome

comment:7 in reply to: ↑ 6 Changed 9 years ago by jinmei

I've updated the branch including the suggestion as well as documentation updates to reflect the discussion.

renaming AbstractZone?: r3745
documentation updates: r3744, r3746

I believe the branch is now okay, and I'm going to merge it to trunk. If you want to make final check, please do so. If you don't, simply ignore this message.

Thanks for the review.

comment:8 Changed 9 years ago by jinmei

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

merged to trunk. btw I forgot to say this but I didn't to spam ChangeLog? for this change (or for #415) since these are intermediate steps for a specific single feature.

closing ticket.

Note: See TracTickets for help on using tickets.