Opened 10 years ago

Closed 10 years ago

#20 closed task (complete)

review Element class

Reported by: jelte Owned by: jelte
Priority: medium Milestone: 01. Running, non-functional authoritative-only server
Component: ~Inter-module communication(obsolete) Version:
Keywords: ccapi 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

The element class will be used to store and pass data in both the command channel (msgq and related supporting classes) and configuration.

The current code resides in branches/parkinglot
The relevant files are
src/lib/cc/cpp/data.cc
src/lib/cc/cpp/data.h

Tests are in
src/lib/cc/cpp/data_unittests.cc (which get added to the run_unittests binary)

the strXML() set of functions aren't finalized (the form of xml hasn't been decided on yet), so you can look at them and comment, but imo these aren't blocking (and not used in the near future).

Subtickets

Attachments (1)

bind10_data_bad_alloc.patch (1.6 KB) - added by jelte 10 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by shane

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

comment:2 Changed 10 years ago by mgraff

  • Component changed from Unclassified to Inter-module communication
  • Keywords ccapi added
  • Milestone set to 01. Running, non-functional authoritative-only server
  • Owner changed from mgraff to jelte

Stream-of-conscience review follows:

(1) Should we hedge against running out of type codes, and add ITEM_FUTURE now? All clients would have to chance, so perhaps the answer is 'bump the protocol version if this is needed later."

(2) Do we really need createFromString? The reason I ask is it is used in exactly one place in the code, and in the tests. The code could probably call the correct create bits and be more efficient (not that it needs to be in the place it is called) and the tests can put the code in the test helper library we'll eventually have to write.

(3) I don't see any tests for the strXML() (or any of the XML bits)

(4) In speed tests I wrote a while back (and I think committed... somewhere...) the C++ code was about as fast as the Python code. I don't think this means we did the python code well, I think it means we did the C++ code poorly. Specifically, the parts I wrote: to/from wire.

comment:3 Changed 10 years ago by jelte

1) I prefer bumping the version

2) I think it is a very useful function, and while i wouldn't mind moving it out of the class itself, having it available for apps would be nice (so they can easily create a 'complex' structure). One of the reasons that it's not used more often is that those instances are in the python code. This function provides a bit of feature-equality :)

3) No, actually i think we should remove them until we decide they're needed (there's also no fromXML), and decide on a schema

4) Yeah. We could pretty easily make the performance a lot better by giving all toWire() functions a stringstream to write to, and make the Element::toWire() a wrapper that calls those.

comment:4 Changed 10 years ago by jelte

  • Owner changed from jelte to mgraff

ok i've removed the toXml() and improved the toWire stuff. During the updates of the configuration system i discovered that i also needed equality checks, so there is now an equals() function in every Element type, and a operator==(ElementPtr?, ElementPtr?).

If you want to take a look at the changes i think we can call this one reviewed.

comment:5 Changed 10 years ago by shane

Bump.

comment:6 Changed 10 years ago by mgraff

  • Owner changed from mgraff to jelte

I question the factory functions. I know they are intended to make them exception-free by catching the memory allocation failure and returning an empty element, but I think this will cause problems. If every method that calls this isn't careful it will send badly formatted messages in the case where memory allocation fails. Isn't it just better to let the application deal with out of memory, perhaps at a higher, request-level event rather than having to validate the return of a factory?

As a general comment, I think we need to re-visit if we need all the specific types for integer, decimal, etc. I know C++ is strongly typed, but we can also specify data types and ranges in the messages we send (documentation good!) and check for those based on received format. In other words, do we really need anything more than the basic 4 types:

hash, array, number, string?

comment:7 Changed 10 years ago by shane

Jelte... bumping again...

comment:8 Changed 10 years ago by jelte

Not sure what to do about the factories, thinking of simply removing the catches there.

Other comment is closely related to whether and when we want to use json instead of isc::data (which needs to be renamed if not removed). But I think we should defer that to another ticket and call this one done (minus the removal of those catches, which is a pretty simple task that I can do later today.

Changed 10 years ago by jelte

comment:9 Changed 10 years ago by jelte

Attached patch removed the bad_alloc catch statements. With that I think this ticket can be closed.

comment:10 Changed 10 years ago by mgraff

  • Status changed from assigned to reviewing

I agree, looks good. I'm assigning this back to you to merge in now? If there's nothing to actually do, just close it; I'm not certain where this ticket really stands in terms of where the code is living today. :)

comment:11 Changed 10 years ago by jelte

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

The main code already lived in trunk. Applied the patch (in r1869), closing ticket. Thanks for the review :)

Note: See TracTickets for help on using tickets.