Opened 8 years ago

Closed 8 years ago

#1252 closed task (complete)

Datasource refactor finishing touch 2: wrappers/NSEC support

Reported by: jelte Owned by: jelte
Priority: low Milestone: Sprint-20111011
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: 2 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The datasource python wrappers (#1179) and several other tasks were developed simultaneously, so any updates to the API may not be reflected in the wrappers yet.

This is one of the tasks to finish that up, most of these should be fairly small or even trivial.

Ticket #1177 added a few more return codes and an extra method, which need to be added to the wrappers too

Subtickets

Change History (12)

comment:1 Changed 8 years ago by jelte

  • Type changed from defect to task

comment:2 Changed 8 years ago by jelte

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

comment:3 Changed 8 years ago by jelte

  • Priority changed from major to minor

comment:4 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 2

comment:5 Changed 8 years ago by jelte

  • Owner set to UnAssigned
  • Status changed from new to reviewing

only open question is whether we need this in the updater too (since that exports all other methods from the finder)

comment:6 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 follow-up: Changed 8 years ago by jinmei

I made one trivial editorial fix (coding guideline thing).

datasrc.cc

This is no worse than the existing code:-), but addClassVariable()
is (at least IMO) error prone. For newer code I'd use
installClassVariable() defined in lib/util/python/pycppwrapper_util.h
in a try-catch block.

Maybe it's beyond the scope of this ticket, but in order to prevent
(or at least discourage) further use of addClassVariable() due to
copy-paste, we should probably at least rename it to something like
obsoletedAndDoNotUseForNewCodeAddClassVariable() until we replace
everything with safer interfaces.

finder_python

  • I believe we can avoid reinterpret_cast here:
        { "find_previous_name", reinterpret_cast<PyCFunction>(ZoneFinder_findPreviousName),
          METH_VARARGS, ZoneFinder_find_doc },/*TODO DOC*/
    
  • As for "TODO DOC": Is it really a TODO? I suspect it's quite easy to generate (though you'll need to make adjustments) from the C++ doc with the semi-auto-generator script. In any case, I don't think referring to ZoneFinder_find_doc is appropriate.
  • Also about documentation, it would be better if we could provide doc for finder result codes (the generator script may not have a support for enums, though)

datasrc_test

  • Unless I misunderstand it findPrevious() throws if it's given a smaller name than zone's origin. I guess this case should be tested too.

about updater

  • probably not related to this ticket, but what's AZoneUpdater_find()? It doesn't seem to be used anywhere.
  • as for whether to define findPreviousName() for the updater, I'm not sure - in practice we probably don't need it. The updater's finder is provided so that the application can perform post-update checks within the transaction, and for that purpose it would be sufficient for a very primitive form of lookup (we probably don't even need GLUEOK etc). On the other hand it would be nicer to provide it for completeness. My slight preference here is to skip it for now in the sense of YAGNI unless/until we see the real need for it.
Last edited 8 years ago by jinmei (previous) (diff)

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

Replying to jinmei:

I made one trivial editorial fix (coding guideline thing).

thanks

datasrc.cc

This is no worse than the existing code:-), but addClassVariable()
is (at least IMO) error prone. For newer code I'd use
installClassVariable() defined in lib/util/python/pycppwrapper_util.h
in a try-catch block.

might as well replace it now, so I did

finder_python

  • I believe we can avoid reinterpret_cast here:
        { "find_previous_name", reinterpret_cast<PyCFunction>(ZoneFinder_findPreviousName),
          METH_VARARGS, ZoneFinder_find_doc },/*TODO DOC*/
    

ok, removed

  • As for "TODO DOC": Is it really a TODO? I suspect it's quite easy to generate (though you'll need to make adjustments) from the C++ doc with the semi-auto-generator script. In any case, I don't think referring to ZoneFinder_find_doc is appropriate.

oops. That TODO was for myself so i wouldn't forget to write it. Then I
forgot...

Did so now.

  • Also about documentation, it would be better if we could provide doc for finder result codes (the generator script may not have a support for enums, though)

They are documented, though perhaps not obviously, in the docstring for find().

I did forget to add the new codes there though. Did so now.

We may indeed want to (automatically) add such things explicitely to the type
docstring at some point though.

datasrc_test

  • Unless I misunderstand it findPrevious() throws if it's given a smaller name than zone's origin. I guess this case should be tested too.

oh, yes, we needed to catch NotImplemented? separately anyway (though this
now reflects the original in that sense, i do wonder whether NotImplemented?
is the right exception for asking the previous name outside of the zone, but
anyway)

about updater

  • probably not related to this ticket, but what's AZoneUpdater_find()? It doesn't seem to be used anywhere.

dead code. removed.

  • as for whether to define findPreviousName() for the updater, I'm not sure - in practice we probably don't need it. The updater's finder is provided so that the application can perform post-update checks within the transaction, and for that purpose it would be sufficient for a very primitive form of lookup (we probably don't even need GLUEOK etc). On the other hand it would be nicer to provide it for completeness. My slight preference here is to skip it for now in the sense of YAGNI unless/until we see the real need for it.

Ok, that's what I figured as well :)

comment:10 in reply to: ↑ 9 Changed 8 years ago by jinmei

Replying to jelte:

The revised code looks okay and ready for merge.

Replying to jinmei:

  • Unless I misunderstand it findPrevious() throws if it's given a smaller name than zone's origin. I guess this case should be tested too.

oh, yes, we needed to catch NotImplemented? separately anyway (though this
now reflects the original in that sense, i do wonder whether NotImplemented?
is the right exception for asking the previous name outside of the zone, but
anyway)

No, it wouldn't be the right exception; it's a short term (TM)
compromise. We'll eventually clarify this when we introduce the
notion of "zone is signed flag". See discussion around here:
http://bind10.isc.org/ticket/1177#comment:18

comment:11 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:12 Changed 8 years ago by jelte

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

Merged this yesterday, closing ticket.

Note: See TracTickets for help on using tickets.