Opened 8 years ago

Closed 7 years ago

#2207 closed task (complete)

define and implement (datasrc::memory::)ZoneUpdater class

Reported by: jinmei Owned by: vorner
Priority: medium Milestone: Sprint-20121023
Component: data source 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: 7 Add Hours to Ticket: 0
Total Hours: 16.55 Internal?: no

Description (last modified by jinmei)

(We probably don't have time to do this for the September release)

A subtask of #2201, depend on #2206.

See the conceptual definition below.

// Provide interfaces for staged zone loading, hiding the underlying
// memory segment model.
class isc::datasrc::memory::ZoneUpdater {
public:
    // (Conceptually) load zone(s) content in memory.  This can take
    // long time.
    // For the local memory segment version, it creates a new ZoneData using
    // the zone table's local memory segment, and load the zone using
    // the given load_action.
    // For the shared memory segment version, this is probably no-op
    // (the time consuming part should have been done by the "b10-memmgr")
    virtual void load() = 0;

    // Install the new zone image so it can actually be used by the caller.
    // For the local memory segment version, it saves the old version of
    // ZoneData from the table and inserts the newly created one  in load()
    // to the zone table.
    // The shared memory version would just internally create
    // a new MemorySegment object with a given shared segment parameter,
    // and pass it to the caller via the install_action.
    virtual void install() = 0;

    // (Conceptually) release any resource used for the older version of zone.
    // This can take long time.
    // The local memory version destroys the saved (in install()) old version
    // of ZoneData.
    // The share memory version would probably be no-op.
    virtual void cleanup() = 0;
};

And implement the ZoneUpdaterLocal derived class:

// local memory segment version of ZoneUpdater
class ZoneUpdaterLocal : public ZoneUpdater {
public:
    ZoneUpdaterLocal(ztable_segment, load_action, install_action);
    ...
};

We also need to define ZoneSegment class, which is supposed to be
used as a parameter of the install_action callback. For the moment we
don't need to use it, so it can be an empty class:

// Eventually this will become an abstract base class
class ZoneSegment {}

Finally, add the getZoneUpdater() method to ZoneTableSegment
(see #2206) and implement it for ZoneTableSegmentLocal.

Subtickets

Change History (19)

comment:1 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 8 years ago by jinmei

  • Description modified (diff)

comment:3 Changed 8 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jelte

  • Milestone set to Sprint-20121009

comment:5 Changed 7 years ago by vorner

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

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

From a quick look, the branch generally seems to do what was intended
in the ticket (except for the install() internal - see below).

But on further thought, I'm now feeling the concept of load_action may
not be really good. To make it work for both the local and shared
segment memory models, the caller (in this case it's ConfigurableClientList)
needs to detect which memory model is used at the time of
ZoneTableSegment::getZoneUpdater() call and provide different
load_action callbacks. It's against the idea of keeping the caller
agnostic about the underlying memory model.

Feature wise, a better behavior would be that we pass all necessary
configuration information to getZoneUpdater(), and each derived
ZoneUpdater class takes care of the rest of the details. In the
case of ZoneUpdaterLocal, it would:

  • it first figures out whether the zone is to be loaded from a file or another data source
  • if it's from a file, extract the file name from the passed config and invoke the parser/loader
  • if it's from another data source, (somehow) get an iterator for the zone of that data source, and load the data provided by the iterator into memory

But then, it may not be easy/clean to implement it without making
ZoneUpdaterLocal know some details of the ConfigurableClientList
internal.

If you can come up with an idea on how to do it cleanly, please make
it happen within this branch (I've not fully thought about it and
haven't had a specific idea). If not, just keep the current design at
the moment and defer the further design cleanup to a later phase.

Some specific initial comments on the current code follow:

  • in retrospect ZoneUpdater (and file name of zone_updater.xx) might not be a good choice because it could be confused with datasrc::ZoneUpdater. If possible, please consider less confusing name.
  • The latest version of #2268 introduced functions to load a zone into memory (either from a file or a zone iterator). It internally creates a ZoneData and returns it with the loaded zone data. This would be used by the load_action callback, so, instead of (creating and) passing a ZoneData to load_action, we'd simply get it from these functions. This means the signature of load_action should be changed by removing ZoneData* from the parameter and changing the return type from void to ZoneData*. I also think we should pass MemorySegment to load_action. At the bottom line these interfaces should be consistent. So, if you think some part of these don't make sense, please discuss it with mukund and make sure these are consistent.
  • I didn't think about what should happen in the ZoneUpdaterLocal destructor, but I think cleaning up any forgotten data makes sense.
  • In ZoneUpdaterLocal::install(), the original intent is to update the zone table within this method (because this is a local-memory-segment specific detail and we want to hide it from ConfigurableClientList). In the local version, at the moment install_action() is mostly meaningless, so we could simply pass NULL.
Last edited 7 years ago by jinmei (previous) (diff)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by vorner

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

Hello

I think it is better to assign it to you, because you've already seen part of
the code, while others would have to review from scratch. If you don't agree
and don't want to review it, feel free to put it to unassigned.

Replying to jinmei:

If you can come up with an idea on how to do it cleanly, please make
it happen within this branch (I've not fully thought about it and
haven't had a specific idea). If not, just keep the current design at
the moment and defer the further design cleanup to a later phase.

I don't think the interface itself is problematic. How I'd see it is:

  • With the Local versions, we pass the load_action, it used to load data into the memory. Just as it is now.
  • In case of the shared memory, the load_action is ignored. The updater (now renamed to reloader) asks the memory manager for the data by the origin and class, then waits for confirmation and maps the shared memory to the address space.
  • In the memory manager, we just reuse the „local“ reloader on top of the shared segment.

Or did I miss something why this couldn't work?

  • in retrospect ZoneUpdater (and file name of zone_updater.xx) might not be a good choice because it could be confused with datasrc::ZoneUpdater. If possible, please consider less confusing name.

Renamed to ZoneReloader?. Is it better?

  • The latest version of #2268 introduced functions to load a zone into memory (either from a file or a zone iterator). It internally creates a ZoneData and returns it with the loaded zone data. This would be used by the load_action callback, so, instead of (creating and) passing a ZoneData to load_action, we'd simply get it from these functions. This means the signature of load_action should be changed by removing ZoneData* from the parameter and changing the return type from void to ZoneData*. I also think we should pass MemorySegment to load_action. At the bottom line these interfaces should be consistent. So, if you think some part of these don't make sense, please discuss it with mukund and make sure these are consistent.

I decided to just change the interface of LoadAction?. It kind of makes sense and feels more natural.

  • In ZoneUpdaterLocal::install(), the original intent is to update the zone table within this method (because this is a local-memory-segment specific detail and we want to hide it from ConfigurableClientList). In the local version, at the moment install_action() is mostly meaningless, so we could simply pass NULL.

Thinking about that, I just removed the install_action completely. There's no
need for the local reloader to take it as a parameter on construction and then
not use it. If there's need for one later on in other reloaders, the factory
function (getZoneReloader) will just get another parameter.

comment:8 Changed 7 years ago by jinmei

I have no problem with reviewing this branch, but my time is up today, so I'll release
it for now in case someone else is willing to do that.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned

comment:10 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

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

Replying to vorner:

If you can come up with an idea on how to do it cleanly, please make
it happen within this branch (I've not fully thought about it and
haven't had a specific idea). If not, just keep the current design at
the moment and defer the further design cleanup to a later phase.

I don't think the interface itself is problematic. How I'd see it is:

  • With the Local versions, we pass the load_action, it used to load data into the memory. Just as it is now.
  • In case of the shared memory, the load_action is ignored. The updater (now renamed to reloader) asks the memory manager for the data by the origin and class, then waits for confirmation and maps the shared memory to the address space.
  • In the memory manager, we just reuse the „local“ reloader on top of the shared segment.

Or did I miss something why this couldn't work?

Hmm, it sounds not as bad as I previously thought. I cannot be sure
until we actually try to cover other cases like the share memory
version, but for now I'm fine with this.

  • in retrospect ZoneUpdater (and file name of zone_updater.xx) might not be a good choice because it could be confused with datasrc::ZoneUpdater. If possible, please consider less confusing name.

Renamed to ZoneReloader?. Is it better?

"*re*loader" still doesn't sound like a good choice to me because
it'll also be used for the initial load (although in that sense
datasrc::ZoneUpdater may be suboptimal, too). ZoneWriter just
occurred to me - how about that? Basically not so different from
"updater" but just to have a different name so it's less confusing.

  • In ZoneUpdaterLocal::install(), the original intent is to update the zone table within this method (because this is a local-memory-segment specific detail and we want to hide it from ConfigurableClientList). In the local version, at the moment install_action() is mostly meaningless, so we could simply pass NULL.

Thinking about that, I just removed the install_action completely. There's no
need for the local reloader to take it as a parameter on construction and then
not use it. If there's need for one later on in other reloaders, the factory
function (getZoneReloader) will just get another parameter.

Okay, but there's still some important gap between what I originally
intended and the actual implementation (probably because the ticket
description wasn't clear enough). See specific comments below.

general

  • on the interface design: maybe just expose the same single method, like doNextStep(), instead of load()/install()/cleanup()? In fact, the caller doesn't have to know what these methods are supposed to do; the only important point for the caller is whether the next step must be considered a critical section (when called in multi-thread environment). So, one idea would be to provide a single method that returns whether there's still more step(s) and in that case whether it must be in a critical section (and the first call would have to be guaranteed to be thread safe). Then the application code would look like this:
        ZoneReloader* reloader;     // get from somewhere
        // result.first = need more step; result.second = need mutex for next step
        std::pair<bool, bool> result(true, false);
        while (result.first) {
            if (result.second) {
                Locker locker(mutex);
                result = reloader->doNextStep();
            } else {
                result = reloader->doNextStep();
            }
        }
    
    This way, we can give the specific derived reloader more freedom of skipping some of the steps or adding extra steps, and neither the reloader nor the application has to worry about most of the call ordering issues. But I'm going to complete my review below based on the current interfaces.
  • this is not in the guideline, but I though we have general agreement about adding a blank line before the \brief comment line:
        /// \throw isc::Unexpected if called second time.
        virtual void load() = 0;
        /// \brief Put the changes to effect.
    
    because without explicit separation it's not immediately clear which method the description is for. I actually would like to propose including this in the guideline.

load_action.h

I'm afraid it's not clear specifically how this type is used just from
this documentation. For example, it doesn't explain how an
implementation of this type gets the source of the zone data
(e.g. zone name, master file name).

zone_reloader.h/cc

  • I'd request derived class implementation provide the strong exception guarantee, i.e, when any of the methods throws, the underlying data should be kept in the state before the method call (either by not making actual change until it's safe or by safely rolling back to the original state) as part of the API contract.
  • I'd hide ZoneReloaderLocal in some a "private" header (not in the public zone_reloader.h)
  • I suspect segment parameter of the ZoneReloaderLocal constructor should be of ZoneTableSegmentLocal, and we can safely do that because this class should be constructed only by a ZoneTableSegmentLocal object.
  • On a related note, ZoneReloaderLocal needs some way to get access to the ZoneTable that should be stored in ZoneTableSegmentLocal because otherwise it cannot update an existing table for reloading one of the zones in it (see also comment on install() below).
  • ZoneReloaderLocal::load() description: this seems to be an older version of description; it doesn't prepare an empty ZoneData by itself anymore. Same for install().
  • ZoneReloaderLocal implementation in general: I'd throw something different from Unexpected to indicate a caller's misbehavior (such as passing an invalid parameter or calling a specific method at a bad timing). Unexpected was intended to mean really unexpected event like an unexpected system error.
  • ZoneReloaderLocal general: are both loaded_ and data_ready_ necessary?
  • ZoneReloaderLocal::install() implementation: the zone table is not expected to be created here, but is passed from the ZoneTableSegmentLocal that created ZoneReloaderLocal (either as ZoneTableSegmentLocal itself or as an explicit parameter).

zone_table_segment_local

  • Not really for this branch, but if we return bare pointer via getZoneReloader(), we should clarify who is responsible for freeing it in the (base class) documentation.

zone_table_segment.h

  • The table member should be set on the construction of ZoneTableHeader, and like things like ZoneData or RdataSet it would only be created and destroyed via separate static create()/destroy() methods that take MemorySegment. (Sorry, I admit this point wasn't clear).
  • getZoneReloader() interface: I'm not sure if it makes sense to pass rrclass to this factory because the zone table segment should know that (at least in theory).

zone_reloader_unittest.cc

  • Does this test the case where we don't call cleanup() to confirm the previous data are still cleaned up?

zone_table_segment_unittest.cc

  • s/load_action/loadAction/

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

  • on the interface design: maybe just expose the same single method, like doNextStep(), instead of load()/install()/cleanup()? In fact, the caller doesn't have to know what these methods are supposed to do; the only important point for the caller is whether the next step must be considered a critical section (when called in multi-thread environment). So, one idea would be to provide a single method that returns whether there's still more step(s) and in that case whether it must be in a critical section (and the first call would have to be guaranteed to be thread safe). Then the application code would look like this:
        ZoneReloader* reloader;     // get from somewhere
        // result.first = need more step; result.second = need mutex for next step
        std::pair<bool, bool> result(true, false);
        while (result.first) {
            if (result.second) {
                Locker locker(mutex);
                result = reloader->doNextStep();
            } else {
                result = reloader->doNextStep();
            }
        }
    
    This way, we can give the specific derived reloader more freedom of skipping some of the steps or adding extra steps, and neither the reloader nor the application has to worry about most of the call ordering issues. But I'm going to complete my review below based on the current interfaces.

I'm not sure about this. It seems to make the use and the interface of the
class more complex. I don't know if we need the flexibility either, we
everything that's in the critical section should happen at once. And there's
some preparation before and some cleanup after. With the fact that any of them
can simply be an empty method, I think we have all the flexibility we need.

Maybe we could rename the load() to something like prepare(). But I kind of
don't see any advantage in your proposal and it makes the code more complex.

  • this is not in the guideline, but I though we have general agreement about adding a blank line before the \brief comment line:

It's not that I wouldn't agree, I just tend to forget O:-).

  • I suspect segment parameter of the ZoneReloaderLocal constructor should be of ZoneTableSegmentLocal, and we can safely do that because this class should be constructed only by a ZoneTableSegmentLocal object.

I tried that. But I don't see any advantage in that either. In theory, it could
work with any other implementation of the class.

  • On a related note, ZoneReloaderLocal needs some way to get access to the ZoneTable that should be stored in ZoneTableSegmentLocal because otherwise it cannot update an existing table for reloading one of the zones in it (see also comment on install() below).

Um, is there a problem with the getHeader().getTable() that's being used in
the install()?

  • ZoneReloaderLocal implementation in general: I'd throw something different from Unexpected to indicate a caller's misbehavior (such as passing an invalid parameter or calling a specific method at a bad timing). Unexpected was intended to mean really unexpected event like an unexpected system error.

In that case, there's a confusion about Unexpected. Grep through the memory/ directory for it and you'll see many occurrences of Unexpected, and none of them looks like a system error.

  • ZoneReloaderLocal general: are both loaded_ and data_ready_ necessary?

Well, the install() „eats“ the data, so data_ready_ becomes false there (so
the old version of the zone data won't get installed again by accident). But
loaded_ stays until the end of life of the class, so we do not try to
override data we have there (either new or old). So these two booleans looked
like an easy way to deal with it. Maybe there could be an enum showing in which
step we are, but I'm not sure if that would make it any more readable.

  • ZoneReloaderLocal::install() implementation: the zone table is not expected to be created here, but is passed from the ZoneTableSegmentLocal that created ZoneReloaderLocal (either as ZoneTableSegmentLocal itself or as an explicit parameter).

The zone table is not being created there. This only gets the pointer to the existing one, so it can be used more conveniently in the following code:

ZoneTable* table(segment_->getHeader().getTable());

zone_table_segment_local

  • Not really for this branch, but if we return bare pointer via getZoneReloader(), we should clarify who is responsible for freeing it in the (base class) documentation.

The base class says:

/// \return New instance of a zone reloader. The ownership is passed
///     onto the caller.

Isn't that explicit enough?

zone_table_segment.h

  • The table member should be set on the construction of ZoneTableHeader, and like things like ZoneData or RdataSet it would only be created and destroyed via separate static create()/destroy() methods that take MemorySegment. (Sorry, I admit this point wasn't clear).

That's why I added the setZoneTable() as a temporary interface only. I expect
the creation will be implemented in some ticket soon, in which point there'll
be no more need for the setZoneTable() method. Or should it be implemented in
this ticket too?

  • getZoneReloader() interface: I'm not sure if it makes sense to pass rrclass to this factory because the zone table segment should know that (at least in theory).

Well, but in practice, it seems not to know. So I can either pass it through
the parameter in this branch or try fixing classes around, but that feels like
out of scope of the ticket.

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

Replying to vorner:

Hello

Replying to jinmei:

  • on the interface design: maybe just expose the same single method, like doNextStep(), instead of load()/install()/cleanup()?

I'm not sure about this. It seems to make the use and the interface of the
class more complex. I don't know if we need the flexibility either, we
everything that's in the critical section should happen at once. And there's
some preparation before and some cleanup after. With the fact that any of them
can simply be an empty method, I think we have all the flexibility we need.

I personally think that it's a plus that the caller doesn't have to
care out method call ordering, but I don't push the idea further.

  • I suspect segment parameter of the ZoneReloaderLocal constructor should be of ZoneTableSegmentLocal, and we can safely do that because this class should be constructed only by a ZoneTableSegmentLocal object.

I tried that. But I don't see any advantage in that either. In
theory, it could work with any other implementation of the class.

Right now, maybe. But I expect if it's other types of
ZoneTableSegment (which uses other types of MemorySegment) it will
have to care about some subtle points due to the difference between
segment types. Then we need to introduce case-analysis based on the
actual derived type of ZoneTableSegment within ZoneWriterLocal.
On the other hand, if and when we can really be sure it's segment-type
independent, I think we should rather make the entire ZoneWriter
class independent from the segment type and make it work in a
polymorphic way in terms of ZoneTableSegment.

  • On a related note, ZoneReloaderLocal needs some way to get access to the ZoneTable that should be stored in ZoneTableSegmentLocal because otherwise it cannot update an existing table for reloading one of the zones in it (see also comment on install() below).

Um, is there a problem with the getHeader().getTable() that's being used in
the install()?

Ah, right. I guess I was somehow confused here (and I certainly
misunderstood the install()).

  • ZoneReloaderLocal implementation in general: I'd throw something different from Unexpected to indicate a caller's misbehavior (such as passing an invalid parameter or calling a specific method at a bad timing). Unexpected was intended to mean really unexpected event like an unexpected system error.

In that case, there's a confusion about Unexpected. Grep through the memory/ directory for it and you'll see many occurrences of Unexpected, and none of them looks like a system error.

I guess many of you were added by yourself:-) but in any case I admit
it hasn't been officially clarified which exception (especially
Unexpected) should be when. We'll probably need a cleanup (and
documentation) ticket.

BTW, I didn't mean it's *only* for system errors. I just meant a
system error is one of the intended cases for this exception. See
below also.

  • ZoneReloaderLocal general: are both loaded_ and data_ready_ necessary?

Well, the install() „eats“ the data, so data_ready_ becomes false there (so
the old version of the zone data won't get installed again by accident). But
loaded_ stays until the end of life of the class, so we do not try to
override data we have there (either new or old). So these two booleans looked
like an easy way to deal with it. Maybe there could be an enum showing in which
step we are, but I'm not sure if that would make it any more readable.

After drawing a state transition diagram with tracking all these
loaded_, data_ready_ and zone_data_, spending some amount of
time, I see the need for these and have some sense the code is
correct. But to be honest I cannot be 100% sure about the correctness
because of the combination of these variables.

I'd probably introduce a single state variable, whose value can be
(maybe INIT and) LOADED, INSTALLED, CLEANED_UP, and only allow the
following transitions:

  • INIT->LOADED->INSTALLED->CLEANED_UP(->destroyed)
  • INIT->LOADED->CLEANED_UP(->destroyed)
  • INIT->CLEANED_UP(->destroyed)
  • INIT->(->destroyed)

It's less flexible about possible transitions (the current
implementation allows INIT->CLEANED_UP->LOADED), but should
cover all practical cases, and would at least reduce one variable to
track.

Can't we do something like this?

(BTW, IMO such issues should be mitigated if we only have doNextStep()
method and controls state transitions completely within the class).

  • ZoneReloaderLocal::install() implementation: the zone table is not expected to be created here, but is passed from the ZoneTableSegmentLocal that created ZoneReloaderLocal (either as ZoneTableSegmentLocal itself or as an explicit parameter).

The zone table is not being created there. This only gets the pointer to the existing one, so it can be used more conveniently in the following code:

ZoneTable* table(segment_->getHeader().getTable());

Yeah, sorry, I misunderstood the code at the time of review. Please
just forget this point.

zone_table_segment_local

  • Not really for this branch, but if we return bare pointer via getZoneReloader(), we should clarify who is responsible for freeing it in the (base class) documentation.

The base class says:

/// \return New instance of a zone reloader. The ownership is passed
///     onto the caller.

Isn't that explicit enough?

Maybe it's enough in practice, but I'd personally be more explicit,
like:

    /// \return New instance of a zone write. The ownership is passed
    ///     onto the caller, and the caller needs to \c delete it when
    ///     it's done with the writer.

because the information on the ownership doesn't necessarily specify
how the object was allocated (and should be destroyed).

zone_table_segment.h

  • The table member should be set on the construction of ZoneTableHeader, and like things like ZoneData or RdataSet it would only be created and destroyed via separate static create()/destroy() methods that take MemorySegment. (Sorry, I admit this point wasn't clear).

That's why I added the setZoneTable() as a temporary interface only. I expect
the creation will be implemented in some ticket soon, in which point there'll
be no more need for the setZoneTable() method. Or should it be implemented in
this ticket too?

Hmm, in retrospect this should have been done in #2206. I guess
Mukund is doing it in #2208 - please confirm that with him. If #2208
is adding it, please just go with setTable() for now, and make sure
it will be removed once #2208 is merged; if not, please discuss it
with him and make sure either #2206 or #2208 does it.

Comments on the revised version follow:

zone_write.h

  • As for the strong guarantee, I'd suggest noting it in the documentation, e.g.:
    /// Each derived class implementation must provide the strong exception
    /// guarantee for each public method.  That is, when any of the methods
    /// throws, the entire in-memory data should roll back to the state at the
    /// time of the call to that method (how to implement it can be implementation
    /// dependent).
    class ZoneWriter {
    

zone_write_local.cc

  • ZoneWriterLocal destructor: I believe we can now remove "Or should we assert instead?"
        // Clean up everything there might be left if someone forgot, just
        // in case. Or should we assert instead?
    
  • ZoneWriterLocal::install: actually, in the second throw would be a good example where Unexpected is better. This case should most likely be an internal bug (not caller's mis-operation), and we could probably even assert() it. But the point of preparing the zone table within the segment is far from this class, using an exception may be a better choice here, and, in that case, that would really be an "unexpected" error.

zone_write_unittest.cc

  • the retry test: I think it's a bit weak for testing the strong guarantee. I'd first load some content to the segment, then trying to load something else with load_throw_ being true, and then confirm the original data is intact.

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:16 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I personally think that it's a plus that the caller doesn't have to
care out method call ordering, but I don't push the idea further.

It is indeed plus, but you exchange it for something far more complicated on
the caller side than keeping the order right (with quite logical names).

In that case, there's a confusion about Unexpected. Grep through the memory/ directory for it and you'll see many occurrences of Unexpected, and none of them looks like a system error.

I guess many of you were added by yourself:-) but in any case I admit
it hasn't been officially clarified which exception (especially
Unexpected) should be when. We'll probably need a cleanup (and
documentation) ticket.

I'm not aware of me adding them, but I might have short memory. Will you create the clean-up ticket, please?

I'd probably introduce a single state variable, whose value can be
(maybe INIT and) LOADED, INSTALLED, CLEANED_UP, and only allow the
following transitions:

[...]

Can't we do something like this?

I did that. It just seems more natural for me to do it the separate variables way, I don't know why, so I usually do that first.

Isn't that explicit enough?

Maybe it's enough in practice, but I'd personally be more explicit,
like:

    /// \return New instance of a zone write. The ownership is passed
    ///     onto the caller, and the caller needs to \c delete it when
    ///     it's done with the writer.

because the information on the ownership doesn't necessarily specify
how the object was allocated (and should be destroyed).

Ah, OK. I added that.

Hmm, in retrospect this should have been done in #2206. I guess
Mukund is doing it in #2208 - please confirm that with him. If #2208
is adding it, please just go with setTable() for now, and make sure
it will be removed once #2208 is merged; if not, please discuss it
with him and make sure either #2206 or #2208 does it.

I asked him if he could do it on merge.

zone_write_unittest.cc

  • the retry test: I think it's a bit weak for testing the strong guarantee. I'd first load some content to the segment, then trying to load something else with load_throw_ being true, and then confirm the original data is intact.

It won't work exactly as you describe, because in the second attempt to load, it complains and won't even call the callback. But I tried something in that direction.

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

Replying to vorner:

The branch looks okay for merge (note: I made one final small cleanup).

One non-blocking comment: if you mean by

    // For some reason it doesn't seem to work by the ZoneNode typedef, using
    // the full definition instead.

that this doesn't compile for you:

    const ZoneNode* node;
    EXPECT_EQ(ZoneTree::EXACTMATCH,

it worked here. So, that's probably compiler dependent.

In that case, there's a confusion about Unexpected. Grep through the memory/ directory for it and you'll see many occurrences of Unexpected, and none of them looks like a system error.

I guess many of you were added by yourself:-) but in any case I admit
it hasn't been officially clarified which exception (especially
Unexpected) should be when. We'll probably need a cleanup (and
documentation) ticket.

I'm not aware of me adding them, but I might have short memory. Will you create the clean-up ticket, please?

Okay.

comment:18 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner
  • Total Hours changed from 0 to 6

comment:19 in reply to: ↑ 17 Changed 7 years ago by vorner

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 6 to 16.55

Hello

Replying to jinmei:

One non-blocking comment: if you mean by

    // For some reason it doesn't seem to work by the ZoneNode typedef, using
    // the full definition instead.

that this doesn't compile for you:

    const ZoneNode* node;
    EXPECT_EQ(ZoneTree::EXACTMATCH,

it worked here. So, that's probably compiler dependent.

Possibly. I think we had a similar problem (that some typedef didn't work
inside the test bodies). I added a note it's for some compilers only.

Thanks, merged and closing.

Note: See TracTickets for help on using tickets.