Opened 7 years ago

Closed 7 years ago

#2211 closed task (complete)

update the data source reconfigure command so it uses thread

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20121106
Component: b10-auth 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: 6 Add Hours to Ticket: 0
Total Hours: 12.5 Internal?: no

Description (last modified by jinmei)

A subtask of #2201, depends on #2203, #2205, #2210.

This task updates the command handler introduced in #2203 so that it now
creates a "reconfigure data source" command for the configurator
thread, and wakes up the thread with it.

Note: according to the ModuleCCSession class description, a remote
module handler is generally expected to be exception free. Make sure
the revised handler won't leak exceptions except for real fatal ones.

When this task completes, data source configuration should now be
possible without disrupting service.

UPDATED: based on the (pre-review) result of #2205 and latest status
of I'd specifically suggest the following approach (I also assume this
ticket will be taken before #2213 because this one has fewer
dependencies).

  • as preparation, allow ModuleCCSession constructor to take functors (not bare function pointers) for config_handler and command_handler, just like we did for addRemoteConfig(). This is actually a few lines of trivial changes (I know that as I did it experimentally)
  • add something like AuthSrv::setDataSrcClientsMgr() and have AuthSrv hold a DataSrcClientsMgr object. AuthSrv somehow calls addRemoteConfig(), whose handler would call something like DataSrcClientsMgr::reconfigureClients() (something like this should be introduced in #2210) on the DataSrcClientsMgr.
  • also add AuthSrv::getDataSrcClientsMgr(), a trivial getter.
  • add an internal Holder class to DataSrcClientsMgr(Base) class like this:
    class DataSrcClientsMgrBase {
        class Holder {
        public:
            Holder(DataSrcClientsMgrBase& mgr) : mgr_(mgr),
                                                 locker_(mgr.map_mutex_)
            {}
            ConfigurableClientList* getDataSrcClientList(zone_class) {
                return (mgr_.clients_map_[zone_class].get());
            }
        private:
            DataSrcClientsMgrBase& mgr_;
            Locker locker_
            
        };
    };
    
    The intent of the Holder class is to hide the concept of locking from the application.
  • then update the following part of LoadZoneCommand::exec():
            isc::util::thread::Mutex::Locker locker(
                server.getDataSrcClientListMutex());
            const boost::shared_ptr<isc::datasrc::ConfigurableClientList>
                list(server.getDataSrcClientList(zone_class));
    
    as follows:
            DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr());
            ConfigurableClientList* datasrc_client_list =
                holder.getDataSrcClientList(zone_class); 
    

Also note: when this ticket is done, we won't have to maintain
AuthSrvImpl::datasrc_client_lists_, and related AuthSrv methods.
They should be cleaned up (and tests should be updated accordingly).

Subtickets

Change History (25)

comment:1 Changed 7 years ago by jelte

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

comment:2 Changed 7 years ago by jelte

  • Milestone Sprint-20120918 deleted

comment:3 Changed 7 years ago by muks

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:5 Changed 7 years ago by jelte

  • Milestone set to Sprint-20121023

comment:6 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:7 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:8 Changed 7 years ago by jinmei

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

comment:9 follow-ups: Changed 7 years ago by jinmei

trac2211 is ready for review. Now at least full data source
reconfigure runs in the background:-)

It depends on #2210, which is now under review, but #2210 seems to be
mostly ready for merge so it should be safe to start reviewing this
branch.

The effective branch point is eb90dff (inclusive); I've incorporated a
snapshot of trac2210 before that, which should be ignored for the
review.

The real new code is small, up to around 0e70ad3. Most of the other
changes are refactoring: updating exiting auth server's code so it
uses the new framework, adjusting test setup, cleanup deprecated
stuff. I believe existing test semantics is (mostly if not all)
preserved, but some are moved to datasrc_clients_mgr_unittest and the
change may seem bigger due to that.

I also fixed the issue of #2363 at commit 6cf9494. It's actually a
small fix, and this branch needs to update the same code, it makes
more sense to do it here at once. I've confirmed the fix manually and
am confident about the fix, but once issues of #2361 are resolved
I'll also confirm it with the lettuce test.

I've not all of the additional things hinted in the tickets as the
branch was getting bigger: I've skipped updating ModuleCCSession
class, and kept the data source config handler still in main.cc.

Since this is a user visible change, I think it deserves a separate
changelog entry. This is the proposed one:

495.?	[func]		team
	b10-auth now handles reconfiguration of data sources in
	background using a separate thread.  This means even if the new
	configuration includes a large amount of data to be loaded into
	memory (very large zones and/or a very large number of zones),
	the reconfiguration doesn't block query handling.
	(Multiple Trac tickets up to #2211)

comment:10 Changed 7 years ago by jinmei

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

comment:11 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

It depends on #2210, which is now under review, but #2210 seems to be
mostly ready for merge so it should be safe to start reviewing this
branch.

While reviewing this branch, I noticed few problems that do not come from this
branch directly (I needed to read a little bit of surrounding code to
understand). I suppose they are from #2210. I'll list them here, but if needed,
I'll forward them or open a new ticket.

This one, I understand the desire to keep the scope of lock as small as
possible, but isn't this a bit to the extreme (note you can .signal() a
condition with locked mutex)?

    void sendCommand(datasrc_clientmgr_internal::CommandID command,
                     data::ConstElementPtr arg)
    {
        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
        }
        cond_.signal();
    }

Is there a need for splice? I think swap is simpler method that does what
we want there.

current_commands.splice(current_commands.end(),
                        *command_queue_);

Also, with swap, we could use a vector instead of list, which might happen to
be slightly faster. But I guess it isn't in a performance-sensitive place.

The body of the thread may throw:

    } catch (const std::exception& ex) {
        // We explicitly catch exceptions so we can log it as soon as possible.
        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED).
            arg(ex.what());
        throw;
    } catch (...) {
        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED);
        throw;
    }

This has two problems. First, the UncaughtException mechanism is a
safe-catch, not meant for a general use. As the documentation says, it should
be avoided to throw from there.

But the bigger problem I see is, if the thread terminates by an exception, the
master thread does not know that until it wait()s for it. The main thread
will just keep sending commands and the queue will grow and grow for ever. So
I'd propose either inform the main thread somehow (but I have no idea how) or
terminate the application if the exception happens (from inside the thread).

The real new code is small, up to around 0e70ad3. Most of the other
changes are refactoring: updating exiting auth server's code so it
uses the new framework, adjusting test setup, cleanup deprecated
stuff. I believe existing test semantics is (mostly if not all)
preserved, but some are moved to datasrc_clients_mgr_unittest and the
change may seem bigger due to that.

The main problem for me was not so much the size as it is scattered around a
lot of places. It took me a while to get a glimpse about what is happening. But
we do need a challenge from time to time O:-).

I've not all of the additional things hinted in the tickets as the
branch was getting bigger: I've skipped updating ModuleCCSession
class, and kept the data source config handler still in main.cc.

Do you want to put them to another ticket, or do it after the second round of
review?

Some more (mostly minor) comments:

Typo („thek“):

+    // Get access to data source client list through the holder and keep thek

Why does it return share pointer if it explicitly says the ownership is in the
class? It is not intuitive ‒ if you get a shared pointer, you'd expect it is
usable as long as you keep it, which is not the case here. If you return an
ordinary pointer here, it might make the caller more careful and read the
documentation.

The part around NULL seems to be badly worded, it doesn't sound English.

+    /// This method simply passes the new configuration to the builder
+    /// and immediately returns.  This method is basically exception free
+    /// as long as the caller a non NULL value for \c config_arg; it doesn't
+    /// validate the argument further.

s/patter/pattern/?

+// Commonly used patter of checking member variables shared between the
+// manager and builder.

I'd expect a method called „swap“ to actually exchange the object with the
parameter. But the swapDataSrcClientLists doesn't modify the parameter, it
keeps the original pointer pointing to the original object. I know the
functionality is not needed, but the name is counter-intuitive. Maybe something
like replaceDataSrcClientList?

There seems to be some relict of re-wrapping lines. The beginning of the string
has a starting { in it's own segment of string, but on the same line. I think
it should be either in the same segment (not ending-quotes, space,
starting-quotes) or on separate lines. It occurs multiple times, like this:

        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

comment:13 in reply to: ↑ 9 Changed 7 years ago by jreed

Replying to jinmei:

I also fixed the issue of #2363 at commit 6cf9494. It's actually a
small fix, and this branch needs to update the same code, it makes
more sense to do it here at once. I've confirmed the fix manually and
am confident about the fix, but once issues of #2361 are resolved
I'll also confirm it with the lettuce test.

branch trac2361 should be ready. (I didn't test it with this trac2211 branch yet though).

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

Thanks for the review.

Replying to vorner:

While reviewing this branch, I noticed few problems that do not come from this
branch directly (I needed to read a little bit of surrounding code to
understand). I suppose they are from #2210. I'll list them here, but if needed,
I'll forward them or open a new ticket.

This one, I understand the desire to keep the scope of lock as small as
possible, but isn't this a bit to the extreme (note you can .signal() a
condition with locked mutex)?

    void sendCommand(datasrc_clientmgr_internal::CommandID command,
                     data::ConstElementPtr arg)
    {
        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
        }
        cond_.signal();
    }

I don't think the current code is "extreme" or what's wrong with it.
But since it's not a performance sensitive path, if you strongly want
that I don't mind putting signal() inside the block either. Right now
I didn't touch the code.

Is there a need for splice? I think swap is simpler method that does what
we want there.

current_commands.splice(current_commands.end(),
                        *command_queue_);

Ah, right, thanks for pointing it out. I replaced it with swap().

Also, with swap, we could use a vector instead of list, which might happen to
be slightly faster. But I guess it isn't in a performance-sensitive place.

Since we use it as a FIFO queue, a vector will make either push or pop
slower, so for now I didn't change that. But as you also pointed out,
such level of performance doesn't matter in this case, so I don't mind
changing it to vector.

The body of the thread may throw:

    } catch (const std::exception& ex) {
        // We explicitly catch exceptions so we can log it as soon as possible.
        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED).
            arg(ex.what());
        throw;
    } catch (...) {
        LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_FAILED_UNEXPECTED);
        throw;
    }

This has two problems. First, the UncaughtException mechanism is a
safe-catch, not meant for a general use. As the documentation says, it should
be avoided to throw from there.

But the bigger problem I see is, if the thread terminates by an exception, the
master thread does not know that until it wait()s for it. The main thread
will just keep sending commands and the queue will grow and grow for ever. So
I'd propose either inform the main thread somehow (but I have no idea how) or
terminate the application if the exception happens (from inside the thread).

Yes, I was aware of that. My original thought (which should have been
mentioned in this ticket) is to handle it in a later ticket. But I'm
also okay with aborting the program (from the thread) when an
unexpected exception happens in the thread in preferring explicit
failure than leaving unworkable state for some unpredictable period.

I've not all of the additional things hinted in the tickets as the
branch was getting bigger: I've skipped updating ModuleCCSession
class, and kept the data source config handler still in main.cc.

Do you want to put them to another ticket, or do it after the second round of
review?

It might even be a subject of discussion whether we want this, but I'm
okay with creating a ticket(s) for this. In any case it's quite
separate from the topic of this ticket, so I'll do it after the
completion of this task.

Why does it return share pointer if it explicitly says the ownership is in the
class? It is not intuitive ‒ if you get a shared pointer, you'd expect it is
usable as long as you keep it, which is not the case here. If you return an
ordinary pointer here, it might make the caller more careful and read the
documentation.

The only reason is that AuthSrvTest wouldn't work otherwise. I've
added some notes on this, and created a ticket so we can eventually
clean this up (#2395).

I'd expect a method called „swap“ to actually exchange the object with the
parameter. But the swapDataSrcClientLists doesn't modify the parameter, it
keeps the original pointer pointing to the original object. I know the
functionality is not needed, but the name is counter-intuitive. Maybe something
like replaceDataSrcClientList?

Hmm, maybe I misunderstand it but I believe this method should
actually swap the contents. That said, the expected usage (some tests
and query_bench) actually doesn't need the "swap" behavior anyway;
they only need to install the specified set of zone data for tests or
benchmarking. So, I'm okay with changing the name to, e.g.,
setDataSrcClientLists() or installDataSrcClientLists() and just
overriding clients_map_:

    void setDataSrcClientLists(datasrc::DataSrcClientListsPtr new_lists) {
        typename MutexType::Locker locker(map_mutex_);
        clients_map_ = new_lists;
    }

if you like it better. Right now I didn't touch the code for this.

There seems to be some relict of re-wrapping lines. The beginning of the string
has a starting { in it's own segment of string, but on the same line. I think
it should be either in the same segment (not ending-quotes, space,
starting-quotes) or on separate lines. It occurs multiple times, like this:

        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

Hmm, I see this one is ill-formatted, but I'm not sure what exactly
you suggested by "either...or...". I also couldn't find other ill
cases by 'grep MasterFiles? *.cc' for tests. Could you be more
specific about your suggestion (or feel free to change it/them to what
you want like to be) and specify other cases you think need to be
fixed?

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:

Replying to vorner:
I don't think the current code is "extreme" or what's wrong with it.
But since it's not a performance sensitive path, if you strongly want
that I don't mind putting signal() inside the block either. Right now
I didn't touch the code.

What I mean is, the code is needlessly complicated even when the benefit is
small. I don't want to push it strongly, just pointing it out it looks
complicated.

Also, with swap, we could use a vector instead of list, which might happen to
be slightly faster. But I guess it isn't in a performance-sensitive place.

Since we use it as a FIFO queue, a vector will make either push or pop
slower, so for now I didn't change that. But as you also pointed out,
such level of performance doesn't matter in this case, so I don't mind
changing it to vector.

Well, you can make both push and shift (pop_front) be amortized O(1). I'm not
sure if stl does that, though. But then, there's no need to repeatedly pop
them. It could look like this (symbolically):

for (ever) {
        vector<Command> commands;
        condition.wait();
        // Empties queue and gets the commands locally
        queue_.swap(commands);
        // No need to pop here or empty the vector, we'll create a new empty one.
        FOREACH(const Command& command, commands) {
                handleCommand;
        }
}

But the bigger problem I see is, if the thread terminates by an exception, the
master thread does not know that until it wait()s for it. The main thread
will just keep sending commands and the queue will grow and grow for ever. So
I'd propose either inform the main thread somehow (but I have no idea how) or
terminate the application if the exception happens (from inside the thread).

Yes, I was aware of that. My original thought (which should have been
mentioned in this ticket) is to handle it in a later ticket. But I'm
also okay with aborting the program (from the thread) when an
unexpected exception happens in the thread in preferring explicit
failure than leaving unworkable state for some unpredictable period.

I think it would be better to put abort in there at least for now, and have
some better error handling deferred for a later ticket (but I don't know what
better should be done).

Hmm, maybe I misunderstand it but I believe this method should
actually swap the contents. That said, the expected usage (some tests
and query_bench) actually doesn't need the "swap" behavior anyway;
they only need to install the specified set of zone data for tests or
benchmarking. So, I'm okay with changing the name to, e.g.,
setDataSrcClientLists() or installDataSrcClientLists() and just
overriding clients_map_:

    void setDataSrcClientLists(datasrc::DataSrcClientListsPtr new_lists) {
        typename MutexType::Locker locker(map_mutex_);
        clients_map_ = new_lists;
    }

if you like it better. Right now I didn't touch the code for this.

My understanding is, the pointers are swapped to point to the other target. But
as the pointer is passed by value, the change to the new_lists is lost when the
function is left.

So I think it should be either propagated out (by passing a reference to a
pointer, for example, and maybe also tested), or changed as you propose.

There seems to be some relict of re-wrapping lines. The beginning of the string
has a starting { in it's own segment of string, but on the same line. I think
it should be either in the same segment (not ending-quotes, space,
starting-quotes) or on separate lines. It occurs multiple times, like this:

        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

Hmm, I see this one is ill-formatted, but I'm not sure what exactly
you suggested by "either...or...". I also couldn't find other ill
cases by 'grep MasterFiles? *.cc' for tests. Could you be more
specific about your suggestion (or feel free to change it/them to what
you want like to be) and specify other cases you think need to be
fixed?

Well, originally, it was like this, I believe:

"{" // The bracket on a separate line
"\"IN\": …

The other possibility is joining the bracket there, so having this:

"{\"IN\": …

I see these occurrences:

    ConstElementPtr reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");
    mgr.reconfigure(reconfigure_arg);
    reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");
    mgr.reconfigure(reconfigure_arg);
    // A valid reconfigure argument
    ConstElementPtr reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

    // On reconfigure(), it just send the RECONFIGURE command to the builder

They are in tests DataSrcClientsMgrTest::reconfigure and DataSrcClientsMgrTest::holder.

comment:17 Changed 7 years ago by jreed

Is it possible to just cherry-pick commit 6cf9494 to fix #2363? (I am not sure the status of #2211.)

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

Replying to vorner:

I don't think the current code is "extreme" or what's wrong with it.
But since it's not a performance sensitive path, if you strongly want
that I don't mind putting signal() inside the block either. Right now
I didn't touch the code.

What I mean is, the code is needlessly complicated even when the benefit is
small. I don't want to push it strongly, just pointing it out it looks
complicated.

I really don't understand it if you mean this one is "extreme" or
complicated:

        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
        }
        cond_.signal();

while this is not:

        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
            cond_.signal();
        }

I don't mind changing the former to the latter, but since I may simply
misunderstand you, I still didn't change that.

Also, with swap, we could use a vector instead of list, which might happen to
be slightly faster. But I guess it isn't in a performance-sensitive place.

Since we use it as a FIFO queue, a vector will make either push or pop
slower, so for now I didn't change that. But as you also pointed out,
such level of performance doesn't matter in this case, so I don't mind
changing it to vector.

Well, you can make both push and shift (pop_front) be amortized O(1). I'm not
sure if stl does that, though. But then, there's no need to repeatedly pop
them. It could look like this (symbolically):

for (ever) {
        vector<Command> commands;
        condition.wait();
        // Empties queue and gets the commands locally
        queue_.swap(commands);
        // No need to pop here or empty the vector, we'll create a new empty one.
        FOREACH(const Command& command, commands) {
                handleCommand;
        }
}

Ah, okay, you're right in that sense. For now, however, I keep the
current code because efficiency in queue operation isn't a big issue
here anyway, and because we may want the pop behavior in future
extensions.

Yes, I was aware of that. My original thought (which should have been
mentioned in this ticket) is to handle it in a later ticket. But I'm
also okay with aborting the program (from the thread) when an
unexpected exception happens in the thread in preferring explicit
failure than leaving unworkable state for some unpredictable period.

I think it would be better to put abort in there at least for now, and have
some better error handling deferred for a later ticket (but I don't know what
better should be done).

Okay, changed. I'll create a separate topic for this issue.

Hmm, maybe I misunderstand it but I believe this method should
actually swap the contents. That said, the expected usage (some tests
and query_bench) actually doesn't need the "swap" behavior anyway;
they only need to install the specified set of zone data for tests or
benchmarking. So, I'm okay with changing the name to, e.g.,
setDataSrcClientLists() or installDataSrcClientLists() and just
overriding clients_map_:

    void setDataSrcClientLists(datasrc::DataSrcClientListsPtr new_lists) {
        typename MutexType::Locker locker(map_mutex_);
        clients_map_ = new_lists;
    }

if you like it better. Right now I didn't touch the code for this.

My understanding is, the pointers are swapped to point to the other target. But
as the pointer is passed by value, the change to the new_lists is lost when the
function is left.

Ah, okay, you're right.

So I think it should be either propagated out (by passing a reference to a
pointer, for example, and maybe also tested), or changed as you propose.

Changed so it just "sets" to the new value.

There seems to be some relict of re-wrapping lines. The beginning of the string
has a starting { in it's own segment of string, but on the same line. I think
it should be either in the same segment (not ending-quotes, space,
starting-quotes) or on separate lines. It occurs multiple times, like this:

        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

Hmm, I see this one is ill-formatted, but I'm not sure what exactly
you suggested by "either...or...". I also couldn't find other ill
cases by 'grep MasterFiles? *.cc' for tests. Could you be more
specific about your suggestion (or feel free to change it/them to what
you want like to be) and specify other cases you think need to be
fixed?

Well, originally, it was like this, I believe:

"{" // The bracket on a separate line
"\"IN\": …

The other possibility is joining the bracket there, so having this:

"{\"IN\": …

I see these occurrences:

    ConstElementPtr reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}],"
        "\"CH\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");
    mgr.reconfigure(reconfigure_arg);
    reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");
    mgr.reconfigure(reconfigure_arg);
    // A valid reconfigure argument
    ConstElementPtr reconfigure_arg = Element::fromJSON(
        "{" "\"IN\": [{\"type\": \"MasterFiles\", \"params\": {},"
        "              \"cache-enable\": true}]}");

    // On reconfigure(), it just send the RECONFIGURE command to the builder

They are in tests DataSrcClientsMgrTest::reconfigure and DataSrcClientsMgrTest::holder.

Okay, thanks. Fixed them at least so that they are not obviously
awkward.

comment:19 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

I really don't understand it if you mean this one is "extreme" or
complicated:

        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
        }
        cond_.signal();

while this is not:

        {
            typename MutexType::Locker locker(queue_mutex_);
            command_queue_.push_back(
                datasrc_clientmgr_internal::Command(command, arg));
            cond_.signal();
        }

I don't mind changing the former to the latter, but since I may simply
misunderstand you, I still didn't change that.

Whenever there is a block that is neither a function or if or a cycle, I find it unusual. But let it be.

Ah, okay, you're right in that sense. For now, however, I keep the
current code because efficiency in queue operation isn't a big issue
here anyway, and because we may want the pop behavior in future
extensions.

OK. It's probably marginal.

I think it would be better to put abort in there at least for now, and have
some better error handling deferred for a later ticket (but I don't know what
better should be done).

Okay, changed. I'll create a separate topic for this issue.

Can you change the LOG_ERROR to LOG_FATAL there, if we are going to abort for that reason?

Otherwise the code looks good, so please merge. Or you might want to wait a
short while, until my local test run finishes, but I don't expect it to break,
there were no changes that should cause that.

comment:21 in reply to: ↑ 20 Changed 7 years ago by jinmei

Replying to vorner:

I really don't understand it if you mean this one is "extreme" or
complicated:

[...]

I don't mind changing the former to the latter, but since I may simply
misunderstand you, I still didn't change that.

Whenever there is a block that is neither a function or if or a cycle, I find it unusual. But let it be.

Ah, okay, I finally understood it. I don't like such blocks either.
On the other hand, in these cases I think it'd good to be somehow
explicit about the lifetime of the lock. So I removed the extra block
and added some comments.

Okay, changed. I'll create a separate topic for this issue.

Can you change the LOG_ERROR to LOG_FATAL there, if we are going to abort for that reason?

Done.

Otherwise the code looks good, so please merge. Or you might want to wait a
short while, until my local test run finishes, but I don't expect it to break,
there were no changes that should cause that.

Assuming the first change is okay, I'll merge the branch once #2210 is
merged.

Thanks,

comment:22 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

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

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

OK, these new changes look good. The tests did run successfully.

comment:24 in reply to: ↑ 23 Changed 7 years ago by jinmei

Replying to vorner:

OK, these new changes look good. The tests did run successfully.

Thanks, merge done, closing.

comment:25 Changed 7 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 3.74 to 12.5
Note: See TracTickets for help on using tickets.