Re: Please review: ticket #48766 Replication changelog can incorrectly skip over updates

[Ludwig Krispenz]

On 05/24/2016 07:00 PM, thierry bordaz wrote:
>  On 05/24/2016 05:24 PM, Ludwig Krispenz wrote:
> >
> > On 05/24/2016 04:20 PM, thierry bordaz wrote:
> >> Hi Ludwig,
> >>
> >> Thanks for your explanation. The design looks very good. I think it 
> >> would be good to put into the code (especially 
> >> clcache_adjust_anchorcsn) the reference to the related design paragraph.
> >>
> >> There is something I do not understand in clcache_skip_change.
> >> My understanding is that this is the only place where the 
> >> consumer_maxcsn is updated.
> >> But there are several conditions, if we decide to skip the update 
> >> that the consumer_maxcsn is not updated.
> >> One of them is 'rid == buf->buf_consumer_rid'.
> >> Does that mean that the consumer_maxcsn remains unchanged for the 
> >> RID of the consumer ?
> >
> > the condition is:
> > if( rid == buf->buf_consumer_rid && buf->buf_ignoreConsumerRID)
> >
> > so it will only be skipped if we determined that we don't need to 
> > send anything for the consumers own rid
>
>  Ok. I get it. thanks.
>
>
>  An other question regarding the update of buf_consumer_ruv.
>  My understanding is that it contains the initial consumer RUV.
>  But later it is used in 
>  clcache_refresh_consumer_maxcsns/clcache_refresh_local_maxcsn to fill 
>  consumer_maxcsn.
>
>  consumer_maxcsn are updated with the non skipped updates to reflect 
>  the current status of the consumer.
>  But when consumer_maxcsns/clcache_refresh_local_maxcsn are called my 
>  understanding is that consumer_maxcsn are reset with buf_consumer_ruv 
>  (initial consumer RUV).
>  Do I get it right ? 
no :-) At least I think I have implemented it differently.
The consumer_maxcsn is set in /clcache_refresh_consumer_maxcsns(), which 
is only called at the first buffer load, not at the relaod.
In //clcache_refresh_local_maxcsns it is only set if we have to add a 
RID to the cscb list, but then no change for this rid was sent before.
/
>  thanks
>  thierry
> >
> >>
> >> thanks
> >> thierry
> >>
> >>
> >>
> >>
> >> On 05/24/2016 09:22 AM, Ludwig Krispenz wrote:
> >>> Hi,
> >>>
> >>> On 05/23/2016 06:29 PM, thierry bordaz wrote:
> >>>>
> >>>>
> >>>> On 05/23/2016 03:06 PM, Ludwig Krispenz wrote:
> >>>>> This is the latest version of the "changelog buffer
> processing" 
> >>>>> fixes.
> >>>>>
> >>>>>
> >>>>> https://fedorahosted.org/389/ticket/48766
> >>>>>
> >>>>>
> https://fedorahosted.org/389/attachment/ticket/48766/0001-reworked-clcach...
>
> >>>>>
> >>>>>
> >>>>> The background for the fix is here, I would like to get feedback 
> >>>>> on this as well to clarify what is unclear
> >>>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta...
>
> >>>>>
> >>>>>
> >>>>
> >>>> Hello Ludwig,
> >>>>
> >>>> I have not yet reviewed the patch. I was looking at the design.
> >>>>
> >>>> Regarding your note: 
> >>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta....
> >>> If you refer to this part:
> >>>
> >>>
> >>>       Special case: RID of the consumer in the current
> >>>       replication session
> >>>
> >>> If the consumer in the replication session is also a master its RID 
> >>> will be contained at least in the consumerRUV. If it is also in the 
> >>> supplier RUV the question is if it should be considered in the 
> >>> decision if updates should be sent. Normally a master has the 
> >>> latest changes applied to itself, so there would be no need to 
> >>> check and send updates for its RID. But there can be scenarios 
> >>> where this is not the case: if the consumer has been restored from 
> >>> an older backup the latest csn for its own RID might be older than 
> >>> changes available on other servers.
> >>>
> >>> |NOTE: The current implementation ignores anchorCSNs based on the consumer
> RID. If, by chance, the anchor csn used is older than this csn, the changes will be sent,
> but they also ca nbe lost.
> >>> |
> >>>
> >>> this referres to the "current" implementation before the fix, the 
> >>> doc started as a post-design doc, and it shoul dbe correctedd
> >>> with the fix the if the supplier has newer changes for the 
> >>> consumerRID than the consumer it will be reflected in the anchor 
> >>> csn calculation.
> >>>
> >>>
> >>>>
> >>>> It is said that the anchorCSN will not be the from the 
> >>>> consumerRID. What is the mechanism that guaranty that the consumer 
> >>>> will receive all the updates it was the originator ?
> >>>>
> >>>> thanks
> >>>> thierry
> >>>> -- 
> >>>> 389-devel mailing list
> >>>> 389-devel(a)lists.fedoraproject.org
> >>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail] 
> >>>>
> >>>
> >>> -- 
> >>> Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> >>> Commercial register: Amtsgericht Muenchen, HRB 153243,
> >>> Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
> >>>
> >>>
> >>> --
> >>> 389-devel mailing list
> >>> 389-devel(a)lists.fedoraproject.org
> >>> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>
> >>
> >>
> >> --
> >> 389-devel mailing list
> >> 389-devel(a)lists.fedoraproject.org
> >> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >
> > -- 
> > Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> > Commercial register: Amtsgericht Muenchen, HRB 153243,
> > Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
> >
> >
> > --
> > 389-devel mailing list
> > 389-devel(a)lists.fedoraproject.org
> > http://lists.fedoraproject.org/admin/lists/[e-mail]
>
>
>
>  --
>  389-devel mailing list
>  389-devel(a)lists.fedoraproject.org
>  http://lists.fedoraproject.org/admin/lists/[e-mail] 
-- 
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael O'Neill

Please review: ticket #48766 Replication changelog can incorrectly skip over updates

[thierry bordaz]

On 05/25/2016 08:47 AM, Ludwig Krispenz wrote:
>  On 05/24/2016 07:00 PM, thierry bordaz wrote:
> >
> >
> > On 05/24/2016 05:24 PM, Ludwig Krispenz wrote:
> >>
> >> On 05/24/2016 04:20 PM, thierry bordaz wrote:
> >>> Hi Ludwig,
> >>>
> >>> Thanks for your explanation. The design looks very good. I think it 
> >>> would be good to put into the code (especially 
> >>> clcache_adjust_anchorcsn) the reference to the related design 
> >>> paragraph.
> >>>
> >>> There is something I do not understand in clcache_skip_change.
> >>> My understanding is that this is the only place where the 
> >>> consumer_maxcsn is updated.
> >>> But there are several conditions, if we decide to skip the update 
> >>> that the consumer_maxcsn is not updated.
> >>> One of them is 'rid == buf->buf_consumer_rid'.
> >>> Does that mean that the consumer_maxcsn remains unchanged for the 
> >>> RID of the consumer ?
> >>
> >> the condition is:
> >> if( rid == buf->buf_consumer_rid && buf->buf_ignoreConsumerRID)
> >>
> >> so it will only be skipped if we determined that we don't need to 
> >> send anything for the consumers own rid
> >
> > Ok. I get it. thanks.
> >
> >
> > An other question regarding the update of buf_consumer_ruv.
> > My understanding is that it contains the initial consumer RUV.
> > But later it is used in 
> > clcache_refresh_consumer_maxcsns/clcache_refresh_local_maxcsn to fill 
> > consumer_maxcsn.
> >
> > consumer_maxcsn are updated with the non skipped updates to reflect 
> > the current status of the consumer.
> > But when consumer_maxcsns/clcache_refresh_local_maxcsn are called my 
> > understanding is that consumer_maxcsn are reset with buf_consumer_ruv 
> > (initial consumer RUV).
> > Do I get it right ?
>  no :-) At least I think I have implemented it differently.
>  The consumer_maxcsn is set in /clcache_refresh_consumer_maxcsns(), 
>  which is only called at the first buffer load, not at the relaod.
>  In //clcache_refresh_local_maxcsns it is only set if we have to add a 
>  RID to the cscb list, but then no change for this rid was sent before.
>  / 
/OK thanks for the explanation.
I was wondering if consumer_maxcsn could go back to buf_consumer_ruv but 
in those cases (first load and first update on known RID) it is right.

Just minor comments on clcache_load_buffer.
/

  * /'rc' should be tested against CLC_STATE_* values not 0.///
  * /if clcache_initial_anchorcsn or clcache_adjust_anchorcsn return
    let's say CLC_STATE_CSN_GT_RUV, in /clcache_load_buffer it will
    return a CLC_STATE_* value not a DB_* value.
  * clcache_refresh_consumer_maxcsns is only called at first load, why
    not including/inlining it into clcache_initial_anchorcsn


Thanks
thierry

//
>  //
> >
> > thanks
> > thierry
> >>
> >>>
> >>> thanks
> >>> thierry
> >>>
> >>>
> >>>
> >>>
> >>> On 05/24/2016 09:22 AM, Ludwig Krispenz wrote:
> >>>> Hi,
> >>>>
> >>>> On 05/23/2016 06:29 PM, thierry bordaz wrote:
> >>>>>
> >>>>>
> >>>>> On 05/23/2016 03:06 PM, Ludwig Krispenz wrote:
> >>>>>> This is the latest version of the "changelog buffer
> processing" 
> >>>>>> fixes.
> >>>>>>
> >>>>>>
> >>>>>> https://fedorahosted.org/389/ticket/48766
> >>>>>>
> >>>>>>
> https://fedorahosted.org/389/attachment/ticket/48766/0001-reworked-clcach...
>
> >>>>>>
> >>>>>>
> >>>>>> The background for the fix is here, I would like to get feedback
>
> >>>>>> on this as well to clarify what is unclear
> >>>>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta...
>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> Hello Ludwig,
> >>>>>
> >>>>> I have not yet reviewed the patch. I was looking at the design.
> >>>>>
> >>>>> Regarding your note: 
> >>>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta....
> >>>> If you refer to this part:
> >>>>
> >>>>
> >>>>       Special case: RID of the consumer in the current
> >>>>       replication session
> >>>>
> >>>> If the consumer in the replication session is also a master its 
> >>>> RID will be contained at least in the consumerRUV. If it is also 
> >>>> in the supplier RUV the question is if it should be considered in 
> >>>> the decision if updates should be sent. Normally a master has the 
> >>>> latest changes applied to itself, so there would be no need to 
> >>>> check and send updates for its RID. But there can be scenarios 
> >>>> where this is not the case: if the consumer has been restored from 
> >>>> an older backup the latest csn for its own RID might be older than 
> >>>> changes available on other servers.
> >>>>
> >>>> |NOTE: The current implementation ignores anchorCSNs based on the 
> >>>> consumer RID. If, by chance, the anchor csn used is older than 
> >>>> this csn, the changes will be sent, but they also ca nbe lost. |
> >>>>
> >>>> this referres to the "current" implementation before the fix,
> the 
> >>>> doc started as a post-design doc, and it shoul dbe correctedd
> >>>> with the fix the if the supplier has newer changes for the 
> >>>> consumerRID than the consumer it will be reflected in the anchor 
> >>>> csn calculation.
> >>>>
> >>>>
> >>>>>
> >>>>> It is said that the anchorCSN will not be the from the 
> >>>>> consumerRID. What is the mechanism that guaranty that the 
> >>>>> consumer will receive all the updates it was the originator ?
> >>>>>
> >>>>> thanks
> >>>>> thierry
> >>>>> -- 
> >>>>> 389-devel mailing list
> >>>>> 389-devel(a)lists.fedoraproject.org
> >>>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail] 
> >>>>>
> >>>>
> >>>> -- 
> >>>> Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> >>>> Commercial register: Amtsgericht Muenchen, HRB 153243,
> >>>> Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham,
> Michael O'Neill
> >>>>
> >>>>
> >>>> --
> >>>> 389-devel mailing list
> >>>> 389-devel(a)lists.fedoraproject.org
> >>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>>
> >>>
> >>>
> >>> --
> >>> 389-devel mailing list
> >>> 389-devel(a)lists.fedoraproject.org
> >>> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>
> >> -- 
> >> Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> >> Commercial register: Amtsgericht Muenchen, HRB 153243,
> >> Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
> >>
> >>
> >> --
> >> 389-devel mailing list
> >> 389-devel(a)lists.fedoraproject.org
> >> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >
> >
> >
> > --
> > 389-devel mailing list
> > 389-devel(a)lists.fedoraproject.org
> > http://lists.fedoraproject.org/admin/lists/[e-mail]
>
>  -- 
>  Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
>  Commercial register: Amtsgericht Muenchen, HRB 153243,
>  Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
>
>
>  --
>  389-devel mailing list
>  389-devel(a)lists.fedoraproject.org
>  http://lists.fedoraproject.org/admin/lists/[e-mail]

Please review: ticket #48766 Replication changelog can incorrectly skip over updates

[Ludwig Krispenz]

On 05/25/2016 10:19 AM, thierry bordaz wrote:
>  On 05/25/2016 08:47 AM, Ludwig Krispenz wrote:
> >
> > On 05/24/2016 07:00 PM, thierry bordaz wrote:
> >>
> >>
> >> On 05/24/2016 05:24 PM, Ludwig Krispenz wrote:
> >>>
> >>> On 05/24/2016 04:20 PM, thierry bordaz wrote:
> >>>> Hi Ludwig,
> >>>>
> >>>> Thanks for your explanation. The design looks very good. I think 
> >>>> it would be good to put into the code (especially 
> >>>> clcache_adjust_anchorcsn) the reference to the related design 
> >>>> paragraph.
> >>>>
> >>>> There is something I do not understand in clcache_skip_change.
> >>>> My understanding is that this is the only place where the 
> >>>> consumer_maxcsn is updated.
> >>>> But there are several conditions, if we decide to skip the update 
> >>>> that the consumer_maxcsn is not updated.
> >>>> One of them is 'rid == buf->buf_consumer_rid'.
> >>>> Does that mean that the consumer_maxcsn remains unchanged for the 
> >>>> RID of the consumer ?
> >>>
> >>> the condition is:
> >>> if( rid == buf->buf_consumer_rid &&
> buf->buf_ignoreConsumerRID)
> >>>
> >>> so it will only be skipped if we determined that we don't need to 
> >>> send anything for the consumers own rid
> >>
> >> Ok. I get it. thanks.
> >>
> >>
> >> An other question regarding the update of buf_consumer_ruv.
> >> My understanding is that it contains the initial consumer RUV.
> >> But later it is used in 
> >> clcache_refresh_consumer_maxcsns/clcache_refresh_local_maxcsn to 
> >> fill consumer_maxcsn.
> >>
> >> consumer_maxcsn are updated with the non skipped updates to reflect 
> >> the current status of the consumer.
> >> But when consumer_maxcsns/clcache_refresh_local_maxcsn are called my 
> >> understanding is that consumer_maxcsn are reset with 
> >> buf_consumer_ruv (initial consumer RUV).
> >> Do I get it right ?
> > no :-) At least I think I have implemented it differently.
> > The consumer_maxcsn is set in /clcache_refresh_consumer_maxcsns(), 
> > which is only called at the first buffer load, not at the relaod.
> > In //clcache_refresh_local_maxcsns it is only set if we have to add a 
> > RID to the cscb list, but then no change for this rid was sent before.
> > /
>
>  /OK thanks for the explanation.
>  I was wondering if consumer_maxcsn could go back to buf_consumer_ruv 
>  but in those cases (first load and first update on known RID) it is right.
>
>  Just minor comments on clcache_load_buffer.
>  /
>
>    * /'rc' should be tested against CLC_STATE_* values not 0./
>  
the statement/
/if (rc == 0)/
/is something  Ididn't change :-), CLC_STATE_READY is 0, so it could be 
changed to /
/if  (rc == CLC_STATE_READY)/
/if you prefer/
/
>    * //
>    * /if clcache_initial_anchorcsn or clcache_adjust_anchorcsn return
>      let's say CLC_STATE_CSN_GT_RUV, in /clcache_load_buffer it will
>      return a CLC_STATE_* value not a DB_* value.
>  
but it will only return CLC_STATE_READY or CLC_STATE_DONE and these are 
checked
>    * clcache_refresh_consumer_maxcsns is only called at first load, why
>      not including/inlining it into clcache_initial_anchorcsn
>  
I was seeing clcache_refresh_consumer_maxcsns() and 
clcache_refresh_local_maxcsns() as parallel as well as 
clcache_*_anchorcsn(). The anchorcsn functions operate on a given state 
of cscb[] and I don't want to mix it.
>  Thanks
>  thierry
>
>  //
> > //
> >>
> >> thanks
> >> thierry
> >>>
> >>>>
> >>>> thanks
> >>>> thierry
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> On 05/24/2016 09:22 AM, Ludwig Krispenz wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 05/23/2016 06:29 PM, thierry bordaz wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 05/23/2016 03:06 PM, Ludwig Krispenz wrote:
> >>>>>>> This is the latest version of the "changelog buffer
> processing" 
> >>>>>>> fixes.
> >>>>>>>
> >>>>>>>
> >>>>>>> https://fedorahosted.org/389/ticket/48766
> >>>>>>>
> >>>>>>>
> https://fedorahosted.org/389/attachment/ticket/48766/0001-reworked-clcach...
>
> >>>>>>>
> >>>>>>>
> >>>>>>> The background for the fix is here, I would like to get 
> >>>>>>> feedback on this as well to clarify what is unclear
> >>>>>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta...
>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> Hello Ludwig,
> >>>>>>
> >>>>>> I have not yet reviewed the patch. I was looking at the design.
> >>>>>>
> >>>>>> Regarding your note: 
> >>>>>>
> http://www.port389.org/docs/389ds/design/changelog-processing-in-repl-sta....
> >>>>> If you refer to this part:
> >>>>>
> >>>>>
> >>>>>       Special case: RID of the consumer in the current
> >>>>>       replication session
> >>>>>
> >>>>> If the consumer in the replication session is also a master its 
> >>>>> RID will be contained at least in the consumerRUV. If it is also 
> >>>>> in the supplier RUV the question is if it should be considered in 
> >>>>> the decision if updates should be sent. Normally a master has the 
> >>>>> latest changes applied to itself, so there would be no need to 
> >>>>> check and send updates for its RID. But there can be scenarios 
> >>>>> where this is not the case: if the consumer has been restored 
> >>>>> from an older backup the latest csn for its own RID might be 
> >>>>> older than changes available on other servers.
> >>>>>
> >>>>> |NOTE: The current implementation ignores anchorCSNs based on the
> consumer RID. If, by chance, the anchor csn used is older than this csn, the changes will
> be sent, but they also ca nbe lost.
> >>>>> |
> >>>>>
> >>>>> this referres to the "current" implementation before the
> fix, the 
> >>>>> doc started as a post-design doc, and it shoul dbe correctedd
> >>>>> with the fix the if the supplier has newer changes for the 
> >>>>> consumerRID than the consumer it will be reflected in the anchor 
> >>>>> csn calculation.
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> It is said that the anchorCSN will not be the from the 
> >>>>>> consumerRID. What is the mechanism that guaranty that the 
> >>>>>> consumer will receive all the updates it was the originator ?
> >>>>>>
> >>>>>> thanks
> >>>>>> thierry
> >>>>>> -- 
> >>>>>> 389-devel mailing list
> >>>>>> 389-devel(a)lists.fedoraproject.org
> >>>>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail] 
> >>>>>>
> >>>>>
> >>>>> -- 
> >>>>> Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> >>>>> Commercial register: Amtsgericht Muenchen, HRB 153243,
> >>>>> Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham,
> Michael O'Neill
> >>>>>
> >>>>>
> >>>>> --
> >>>>> 389-devel mailing list
> >>>>> 389-devel(a)lists.fedoraproject.org
> >>>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> 389-devel mailing list
> >>>> 389-devel(a)lists.fedoraproject.org
> >>>>
> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>>
> >>> -- 
> >>> Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> >>> Commercial register: Amtsgericht Muenchen, HRB 153243,
> >>> Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
> >>>
> >>>
> >>> --
> >>> 389-devel mailing list
> >>> 389-devel(a)lists.fedoraproject.org
> >>> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >>
> >>
> >>
> >> --
> >> 389-devel mailing list
> >> 389-devel(a)lists.fedoraproject.org
> >> http://lists.fedoraproject.org/admin/lists/[e-mail]
> >
> > -- 
> > Red Hat GmbH,http://www.de.redhat.com/, Registered seat: Grasbrunn,
> > Commercial register: Amtsgericht Muenchen, HRB 153243,
> > Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael
> O'Neill
> >
> >
> > --
> > 389-devel mailing list
> > 389-devel(a)lists.fedoraproject.org
> > http://lists.fedoraproject.org/admin/lists/[e-mail]
>
>
>
>  --
>  389-devel mailing list
>  389-devel(a)lists.fedoraproject.org
>  http://lists.fedoraproject.org/admin/lists/[e-mail] 
-- 
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Paul Argiry, Charles Cachera, Michael Cunningham, Michael O'Neill