List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:April 16 2008 10:40am
Subject:Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259
View as plain text  
Hi Rafal,

thanks again for your comments. And sorry for quoting the full email. I
believe you mentioned it as your preference. Please correct me if I
misunderstood.

Please find comments inline.

Rafal Somla, 14.04.2008 19:18:
> Hi Ingo,
> 
> Thank you for the explanations and sorry for the delay. After reading
> your explanations there are only two issues left to be settled.
> 
> 1. I still don't understand a piece of the code. See the remarks about
> this comment: "/* If HIT_LIMIT is 1, return error to suppress send_ok().
> */" and the following code.

Does the following make a better explanation?

  /* Execute the special sync point 'now' if activated above. */
  if (is_dsp_now)
  {
    DEBUG_SYNC(thd, "now");
    /*
      If HIT_LIMIT for sync point "now" was 1, the execution of the sync
      point decremented it to 0. In this case the following happened:

      - an error message was reported with my_error() and
      - the statement was killed with thd->killed= THD::KILL_QUERY.

      If a statement reports an error, it must not call send_ok().
      The calling functions will not call send_ok(), if we return TRUE
      from this function.

      To detect, if an error message has been reported, we can check one
      of the above conditions. That is, either examine the error message
      stack for an entry, or check thd->killed. We do the latter below.
    */
    if (thd->killed)
      DBUG_RETURN(TRUE);
  }


> 
> 2. About the st_debug_sync_action structure which is moved around with
> memmove: for safety I propose to add a comment explaining potential
> problems with constructors/destructors.

I changed the declaration to:

/*
  Action to perform at a synchronization point.
  NOTE: This structure is moved around in memory by realloc(), qsort(),
        and memmove(). Do not add objects with non-trivial constuctors
        or destructors, which might prevent moving of this structure
        with these functions.
*/
struct st_debug_sync_action
{
  ...
};

> 
> After resolving these I'll accept the patch for pushing.
> 
> See also my comments inlined below.
> 
> Rafal
> 
> Ingo Strüwing wrote:
>> Hi Rafal,
>>
>> thanks for the review.
>>
>> Please do not quote huge emails in full. I had to save the message to
>> find your comments with an editor. :-(
>>
> 
> Sorry for inconvenience. I personally prefer to see the full context
> when reading comments but I'll try to remember your preference.

Thanks. I'll try to remember your preference as well. :-)

> 
>> Please find replies below.
>>
>> Rafal Somla, 20.03.2008 11:19:
>> ...
>>> Ingo Struewing wrote:
>> ...
>>>> ChangeSet@stripped, 2008-03-11 18:54:28+01:00, istruewing@stripped
>>>> +32 -0
>>>>   WL#4259 - Debug Sync Facility
>>
>> ...
>>>> +/* Debug sync control. Referenced by THD. */
>>>> +struct st_debug_sync_control
>>>> +{
>>>> +  st_debug_sync_action  *ds_action;             /* array of actions */
>>> Wouldn't it be nicer to declare it as "st_debug_sync_action
>>> ds_action[]"?
>>
>> Maybe a matter of taste. Your form would look to me as an in-place
>> array, not a pointer to the array somewhere in memory.
>>
>> Anyway I tried it and had problems to cast the malloc() result into it.
> 
> Ok, leave it as it is.
> 
>>
>>> Another thing - why not use dynamic array here and reuse the code from
>>> mysys library?
>>
>> The key is performance. I could get approval for string sync points only
>> after proving that the cost is not too high. The main time consumer is
>> the binary search in that array. It would become much bigger if I had to
>> access the array elements via get_dynamic().
>>
>> Also, I think, the code for managing this array is pretty small. It
>> won't profit much from changing to dynamic array.
>>
> 
> Ok.
> 
>> ...
>>>> +/**
>>>> +  Callback from wait_for_lock() for debug sync. See thr_lock.c.
>>>> +
>>>> +  @description
>>>> +    One can use this to signal when a thread is going to wait for a
>>>> lock.
>>> I was bit confused looking at this callback function. Perhaps in the
>>> comment you could elaborate more on why you define it here and how it is
>>> used.
>>
>> Ok. I added:
>>
>>     We cannot place a sync point directly in thr_lock.c. It is C code
>>     and does not include mysql_priv.h. So it does not know the macro
>>     DEBUG_SYNC(thd, sync_point_name). The macro needs a 'thd' argument.
>>     Hence it cannot be used in files outside of the sql/ directory.
>>
>>     The workaround is to call back simple functions like this one from
>>     non-sql/ files.
>>
>>     We want to allow modules like thr_lock to be used without sql/ and
>>     especially without Debug Sync. So we cannot just do a simple call
>>     of the callback function. Instead we provide a global pointer in
>>     the other file, which is to be set to the callback by Debug Sync.
>>     If the pointer is not set, no call back will be done. If Debug
>>     Sync sets the pointer to a callback function like this one, it will
>>     be called. That way thr_lock.c does not have an undefined reference
>>     to Debug Sync and can be used without it. Debug Sync, in contrast,
>>     has an undefined reference to that pointer and thus requires
>>     thr_lock to be linked too. But this is not a problem as it is part
>>     of the MySQL server anyway.
>>
> 
> That is very extensive explanation...
> 
>>>> +
>>>> +  @note
>>>> +    Beware of waiting for a signal here. The lock has aquired its
>>>> mutex.
>>>> +    While waiting on a signal here, the locking thread could not
>>>> aquire
>>>> +    the mutex to release the lock. One could lock up the table
>>>> +    completely.
>>> Hmm, I don't understand this note. But perhaps it's only me...
>>
>> Ok. I added:
>>
>>     In detail it works so: When thr_lock() tries to acquire a table
>>     lock, it locks the lock->mutex, checks if it can have the lock, and
>>     if not, it calls wait_for_lock(). Here it unlocks the table lock
>>     while waiting on a condition. The sync point is located before this
>>     wait for condition. If we have a waiting action here, we hold the
>>     the table locks mutex all the time. Any attempt to look at the table
>>     lock by another thread blocks it immediately on lock->mutex. This
>>     can easily become an unexpected and unobvious blockage. So be
>>     warned: Do not request a WAIT_FOR action for the 'wait_for_lock'
>>     sync point unless you really know what you do.
>>
> 
> Ok, perfect.
> 
>> ...
>>>> +  for (; action < action_end; action++)
>>>> +  {
>>>> +    action->signal.free();
>>>> +    action->wait_for.free();
>>>> +    action->sync_point.free();
>>>> +  }
>>>> +  DBUG_VOID_RETURN;
>>>> +}
>>>> +
>>> It is just a suggestion, but adding constructor/destructor to
>>> st_debug_sync_action structure would mke the code look nicer and easier
>>> to maintain IMO.
>>
>> Interesting idea. But how would I call the destructor here? I do not
>> create the objects with 'new'. So I cannot 'delete' them. For efficiency
>> I want to allocate and free the actions in bunches.
>>
>> A compromise might be an action->free() method that includes the above.
>>
> 
> Constructors and destructors are called not only for dynamically
> allocated objects but also for ones declared in the usual way.
> 
> Anyway, it was just an idea - if you don't like it it is ok to go
> without changes here.
> 
>> ...
>>>> +static st_debug_sync_action *debug_sync_find(st_debug_sync_action
>>>> *actionarr,
>>>> +                                             int quantity,
>>>> +                                             const char* dsp_name,
>>>> +                                             uint name_len)
>>>> +{
>>>> +  st_debug_sync_action  *action;
>>>> +  int                   low ;
>>>> +  int                   high ;
>>>> +  int                   mid ;
>>>> +  int                   diff ;
>>>> +
>>>> +  low= 0;
>>>> +  high= quantity;
>>>> +
>>>> +  while (low < high)
>>>> +  {
>>>> +    mid= (low + high) / 2;
>>>> +    action= actionarr + mid;
>>> What if actionarr is NULL?
>>
>> It will crash. Just like most of MySQL functions will crash if called
>> with a NULL 'thd' argument.
>>
>> In other words, before calling debug_sync_find() I always check
>> ds_control->ds_active. If the algorithms in this file are correct,
>> 'ds_active' can be non-zero only after an action array has been
>> successfully allocated. If they are not correct, we are lost anyway. ;-)
>> I'll add a DBUG_ASSERT().
>>
> 
> I think it is a good idea to add DBUG_ASSERT() for each pointer we want
> to dereference. In case of errors (and we all make mistakes) it makes it
> much easier to detect the cause of failure.
> 
>> ...
>>>> +static void debug_sync_reset(THD *thd)
>>>> +{
>>>> +  st_debug_sync_control *ds_control= thd->debug_sync_control;
>>>> +  DBUG_ENTER("debug_sync_reset");
>>>> +
>>>> +  /* Remove all actions of this thread. */
>>>> +  ds_control->ds_active= 0;
>>> What if ds_control is NULL?
>>
>> Same question, same answer. :)
>>
>> But how can it become NULL? At thread start it is allocated, at thread
>> end deallocated. If allocation fails, the whole facility is disabled.
>> debug_sync_reset() will never be called then. Is there any other place
>> where thd->debug_sync_control can become NULL?
>>
> 
> Sure, I understand all these assumptions. But DBUG_ASSERT() is there to
> verify such assumptions...
> 
>>>> +
>>>> +  /* Clear the global signal. */
>>>> +  debug_sync_global.ds_signal.length(0);
>>>> +
>>> Don't we need to call String destructors for all stored actions?
>>
>> It depends.
>>
>> 1. debug_sync_global.ds_signal is not an action. It is the container for
>> the signal sent by the last SIGNAL action.
>>
>> 2. The idea with all the 'String' objects is to keep the allocated
>> memory for the whole life time of the 'String' object to reduce the
>> number of allocations/frees.
>>
>> String::length(0) means that the string length is set to zero, but the
>> allocated memory is kept for later reuse. String::copy() does not need
>> to do a new allocation if the new string is not longer than the longest
>> string assigned to this object before.
>>
>> Freeing of the allocated memory is only done before the enclosing
>> container (action or debug_sync_global) is freed. This happens at thread
>> end for all actions and at server end for debug_sync_global.ds_signal.
>>
> 
> Right, in dbug_sync_end() and in debug_sync_free_actions(). Now I see
> these.
> 
>> One exception is the compression of the actions array when an action in
>> the middle between other actions is removed. This is done by shifting
>> the later actions down one slot. This overwrites the string pointers of
>> the String objects inside the removed action. To avoid loss of the
>> allocated memory, I free it first. After the shift, the last action is
>> duplicated. This includes the string pointers. I fix this by bzeroing
>> the now unused action (including the string pointers).
> 
> Correct, I see the debug_sync_free_actions() call in
> debug_sync_remove_action().
> 
>>
>> At the time of this writing I noticed that I could avoid the free at
>> this point if I copy the to-be-removed action to a temporary variable,
>> do the shift, and then copy the saved action back to the now free
>> position.
>>
>> ...
>>>> +static void debug_sync_remove_action(st_debug_sync_control
>>>> *ds_control,
>>>> +                                     st_debug_sync_action *action)
>>>> +{
>>>
>>> DBUG_ASSERT(ds_control) ?
>>> DBUG_ASSERT(action) ?
>>
>> Ok. Will add. Plus:
>>   DBUG_ASSERT(ds_control == current_thd->debug_sync_control);
>>
>> But please note that most of the functions in this file are static.
>> Their use is an internal affair. The risk of misuse is limited.
>>
> 
> I think it never hurts to make our assumptions explicit if it is
> possible. Even for static functions which could be used by other
> developers working on that code. For example when I was working with
> replication code I was using static helper functions a lot, and since
> they were not well documented the risk of errors was high...
> 
>>>> +  uint dsp_idx= action - ds_control->ds_action;
>>> This code will break if we store pointer to an action, then add new
>>> actions to ds_control->ds_action so that it is re-allocated to a new
>>> location and then try to remove stored action... Should be documented as
>>> precondition for this function or... a safer implementation used.
>>
>> It is always a risk to store a pointer to something that can be deleted,
>> reallocated or otherwise become invalid.
>>
>> If we store a pointer to an action over code that can reallocate
>> ds_action, then all later uses of that pointer is broken from the
>> beginning. There is nothing special with debug_sync_remove_action().
>>
> 
> Ok, perhaps it is enough to use an implicit assumption that all passed
> pointers are valid. A safer option would be to pass index of the action
> instead of the pointer. But it is ok to leave it as implemented now.
> 
>> But if we can have a safer implementation with same or better
>> performance, that would be good anyway.
>>
>> ...
>>>> +    /* Free strings before copying them over. */
>>>> +    debug_sync_free_actions(action, 1);
>>>> +
>>>> +    /* Move actions down. */
>>>> +    memmove(ds_control->ds_action + dsp_idx,
>>>> +            ds_control->ds_action + dsp_idx + 1,
>>>> +            (ds_control->ds_active - dsp_idx) *
>>>> +            sizeof(st_debug_sync_action));
>>>> +
>>>> +    /* Remove double references of String pointers. */
>>>> +    bzero(ds_control->ds_action +
>>>> +          ds_control->ds_active, sizeof(st_debug_sync_action));
>>> To my taste it is too dangerous to move constructed objects like this.
>>> My objections are:
>>>
>>> - I'm not sure if C++ compilers guarantee (and will guarantee that in
>>> the future) that if x is an object of type X and you move sizeof(X)
>>> bytes from &x to a different addres b then you will get a valid instance
>>> of X object at b
>>>
>>> - In the future one can change either st_debug_sync_action or String
>>> class. A possible change is to add member with non-trivial copy
>>> constructor (e.g. with reference couts). Then above code will break
>>> things.
>>
>> In theory you might be right.
>>
>>> Btw. Have you considered using some pointer structure (binary search
>>> tree or even a simple linked list) for storing actions. Then there will
>>> be no need to move objects around.
>>
>> Yes, I did. But that would introduce an additional layer of indirection
>> in the binary search algorithm, which is the most performance critical
>> piece of this patch.
>>
>> But I could replace String by something handcrafted that doesn't use
>> constructors.
> 
> That wont help because later, when the code is developed by others,
> someone can extend st_dbug_sync_action with other members and we'll get
> a hard to detect problems.
> 
> How about such compromise: add comment to the documentation of
> st_debug_sync_action structure saying that it is moved around using
> memmove and therefore can not contain members which have non-trivial
> constructors or destructors.

I hope I addressed it sufficiently with my comment near the top of this
email.

> 
>>
>> ...
>>>> +static st_debug_sync_action *debug_sync_get_action(THD *thd,
>>>> +                                                   const char
>>>> *dsp_name,
>>>> +                                                   uint name_len)
>>>> +{
>>>> +  st_debug_sync_control *ds_control= thd->debug_sync_control;
>>>> +  st_debug_sync_action  *action;
>>>> +  DBUG_ENTER("debug_sync_get_action");
>>>> +  DBUG_PRINT("debug_sync", ("sync_point: '%.*s'", (int) name_len,
>>>> dsp_name));
>>>> +
>>> DBUG_ASSERT(ds_control) ?
>>
>> Ok.
>>
>> ...
>>>> +      void *new_action= my_realloc(ds_control->ds_action,
>>>> +                                   new_alloc *
>>>> sizeof(st_debug_sync_action),
>>>> +                                   MYF(MY_WME | MY_ALLOW_ZERO_PTR));
>>> Is it the case that if my_realloc moves the memory to a different
>>> location it copies all the bytes from the old location?
>>
>> Yes.
>>
>> ...
>>>> +/**
>>>> +  Set a debug sync action.
>> ...
>>>> +  @description
>>>> +    This is called from the debug sync parser.
>>> Perhaps elaborate more what does that mean to "set an action".
>>
>> I changed:
>>
>>   @description
>>     This is called from the debug sync parser. It arms the action for
>>     the requested sync point. If the action parsed into an empty action,
>>     it is removed instead.
>>
>>     Setting an action for a sync point means to make the sync point
>>     active. When it is hit it will execute this action.
>>
>>     Before parsing, we "get" an action object. This is placed at the
>>     end of the thread's action array unless the requested sync point
>>     has an action already.
>>
>>     Then the parser fills the action object from the request string.
>>
>>     Finally the action is "set" for the sync point. If it was parsed
>>     to be empty, it is removed from the array. If it did belong to a
>>     sync point before, the sync point becomes inactive. If the action
>>     became non-empty and it did not belong to a sync point before (it
>>     was added at the end of the action array), the action array needs
>>     to be sorted by sync point.
>>
>>     If the sync point name is "now", it is executed immediately.
>>
> 
> Ok, that is more than I expected...
> 
>>>> +*/
>>>> +
>>>> +static bool debug_sync_set_action(THD *thd, st_debug_sync_action
>>>> *action)
>>>> +{
>>>> +  st_debug_sync_control *ds_control= thd->debug_sync_control;
>>>> +  bool is_dsp_now= FALSE;
>>>> +  DBUG_ENTER("debug_sync_set_action");
>>>> +  DBUG_ASSERT(ds_control);
>>> You also assume action != NULL.
>>
>> Ok.
>>
>> ...
>>>> +    DEBUG_SYNC(thd, "now");
>>>> +    /* If HIT_LIMIT is 1, return error to suppress send_ok(). */
>>> I don't understand the above comment...
>>
>> I changed to:
>>
>>       If HIT_LIMIT for sync point "now" was 1, the execution of the sync
>>       point decremented it to 0. In this case an error message was
>>       reported and the statement killed.
>>
>>       If a statement reports an error, it must not call send_ok().
>>
>>       To suppress a call to send_ok(), we return TRUE from this
>>       function. The killed statement is the indication for the hit limit
>>       error.
>>
>> ...
> 
> But how all this relates to the code following the comment, which is
> 
> +    if (thd->killed)
> +      goto err;
> 
> I don't see why you check thd->killed here and how it relates to
> HIT_LIMIT...

I hope I addressed it sufficiently with my comment near the top of this
email.

> 
>>>> +static char *debug_sync_token(char **token_p, uint *token_length_p,
>>>> char *ptr)
>>>> +{
>>> How about asserting here that token_p, token_length_p and ptr are not
>>> NULL? The code below uses that assumption.
>>
>> Ok. I'll also add DBUG_ASSERT(thd) to all functions that use thd. And
>> asserts to all pointer input parameters of all functions.
>>
>> Do you write your own code so that you assert everything? I didn't have
>> the impression when reviewing one of your patches recently.
>>
> 
> Yes, I try to DBUG_ASSERT() all pointers which I dereference. If you
> spot a place where I forgot to do that please let me know!
> 
>> ...
>>>> +/**
>>>> +  Evaluate a debug sync action string.
>> ...
>>>> +  @description
>>>> +    This is called when the DEBUG_SYNC system variable is set.
>>>> +    Parse action string, build a debug sync action, activate it.
>>> Mention that the result of parsing is stored inside thd's action table.
>>
>> I added:
>>
>>     Before parsing, we "get" an action object. This is placed at the
>>     end of the thread's action array unless the requested sync point
>>     has an action already.
>>
>>     Then the parser fills the action object from the request string.
>>
>>     Finally the action is "set" for the sync point. This means that the
>>     sync point becomes active or inactive, depending on the action
>>     values.
>>
> 
> Ok.
> 
>>>> +
>>>> +  @note
>>>> +    The input string needs to be ASCII NUL ('\0') terminated. We split
>>>> +    nul-terminated tokens in it without copy.
>>> Perhaps add here all the other assumptions about action_str. For
>>> example, that it must be in system character set.
>>
>> I added:
>>
>>     For more constraints for the string see the function comment of
>>     debug_sync_token().
>>
>> So I avoided "comment duplication". ;-)
>>
> 
> Sure, that is enough, except... that there is @see doxygen keyword which
> would be good to use here so that a reader of doxygen docs can click on it.

Is this better?

  @see the function comment of debug_sync_token() for more constraints
    for the string.

> 
>>>> +*/
>>>> +
>>> This is the grammar for command strings which I deduced from
>>> reverse-engenieering the code (there are additional constraints, for
>>> example EXECUTE must be preceeded by SIGNAL or WAIT_FOR:
>>>
>>> RESET
>>> <point name> TEST
>>> <point name> CLEAR
>>> <point name> [SIGNAL <signal name>] [WAIT_FOR <signal name>
> [TIMEOUT
>>> <seconds>]]
>>>              [EXECUTE <count>] [HIT_LIMIT <count>]
>>>
>>> Would be good if this grammar is documented somewhere. In particular,
>>> the order of actions is fixed which is not clearly documented in the
>>> existing documentation.
>>
>>
>> The big comment at top of debug_sync.cc explains the syntax, though not
>> in the from you suggested. Also it points to the following resources,
>> which repeat the syntax explanation:
>>
>>   ==== Further reading ====
>>
>>   For a discussion of other methods to synchronize threads see
>>   http://forge.mysql.com/wiki/MySQL_Internals_Test_Synchronization
>>
>>   For complete syntax tests, functional tests, and examples see the test
>>   case debug_sync.test.
>>
>>   See also worklog entry WL#4259 - Test Synchronization Facility
>>
>> I added:
>>
>>   ==== Formal Syntax ====
>>
>>   The string to "assign" to the DEBUG_SYNC variable can contain:
>>
>>   {RESET |
>>    <sync point name> TEST |
>>    <sync point name> CLEAR |
>>    <sync point name> {{SIGNAL <signal name> |
>>                        WAIT_FOR <signal name> [TIMEOUT <seconds>]}
>>                       [EXECUTE <count>] &| HIT_LIMIT <count>}
>>
>>   Here '&|' means 'and/or'. This means that one of the sections
>>   separated by '&|' must be present or both of them.
>>
> 
> Yes, I read the documentation. My point was mainly that the fixed order
> of clauses was not documented. I also prefer formal grammar description
> for its clarity.
> 
>>>> +static bool debug_sync_eval_action(THD *thd, char *action_str)
>>> How about calling it debug_sync_parse_action instead?
>>
>> It does not only parse the string, but also [de-]activate the sync point
>> or executes other functions. "Evaluate" is usually something like parse
>> + execute.
>>
> 
> Ok.
> 
>>>> +{
>>>> +  st_debug_sync_action  *action= NULL;
>>>> +  const char            *errmsg;
>>>> +  char                  *ptr;
>>>> +  char                  *token;
>>>> +  uint                  token_length;
>>>> +  DBUG_ENTER("debug_sync_eval_action");
>>>> +
>>>> +  /*
>>>> +    Get debug sync point name. Or a special command.
>>>> +  */
>>>> +  if (!(ptr= debug_sync_token(&token, &token_length,
> action_str)))
>>>> +  {
>>>> +    errmsg= "Missing synchronization point name";
>>> Why error messages are not internationalized?
>>
>> Too many new error messages for a debugging facility. Experts, who are
>> able to make use of this facility, should know English language. We
>> often use such combinations of of an internationalized error message
>> (here ER_PARSE_ERROR) with English details. See for example
>> mysql_binlog_send().
>>
> 
> Ok - I buy this argument.
> 
>> ...
>>>> +  /*
>>>> +    Now check for actions that define a new action.
>>>> +    Initialize action. Do not use bzero(). Strings may have malloced.
>>>> +  */
>>>> +  action->activation_count= 0;
>>>> +  action->hit_limit= 0;
>>>> +  action->execute= 0;
>>>> +  action->timeout= 0;
>>>> +  action->signal.length(0);
>>>> +  action->wait_for.length(0);
>>>> +
>>> Use of constructor would be nice...
>>
>> But I don't construct action here. I initialize it. And only part of it.
>>
> 
> Ok, let's leave it as it is.
> 
>> ...
>>>> +static void debug_sync_execute(THD *thd, st_debug_sync_action *action)
>>>> +{
>> ...
>>>> +  if (action->execute)
>>>> +  {
>> ...
>>>> +    action->execute--;
>>> Wouldn't it be better to decrease the counter at the very beginning?
>>> This way we don't have to worry about early returns and such...
>>
>> Ok.
>>
>> ...
>>>> +++ b/sql/mysql_priv.h    2008-03-11 18:54:24 +01:00
>> ...
>>>> +/* Command line option --debug-sync. See mysqld.cc. */
>>>> +extern uint opt_debug_sync_timeout;
>>> Comment should say "--debug-sync-timeout".
>>
>> Ok.
>>
>> ...
>>
>> Regards
>> Ingo
> 

Regards
Ingo
-- 
Ingo Strüwing, Senior Software Developer
MySQL GmbH, Dachauer Str. 37, D-80335 München
Geschäftsführer: Kaj Arnö - HRB München 162140
Thread
bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Struewing12 Mar
  • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing12 Mar
  • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Sergei Golubchik12 Mar
    • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing12 Mar
      • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Sergei Golubchik17 Mar
        • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing17 Mar
          • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Sergei Golubchik17 Mar
            • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing18 Mar
  • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Rafal Somla20 Mar
    • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing10 Apr
      • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Rafal Somla14 Apr
        • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Ingo Strüwing16 Apr
          • Re: bk commit into 6.0 tree (istruewing:1.2569) WL#4259Rafal Somla16 Apr