Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6812004:
sync: Make nudge + config jobs reliable in SyncerThread2 (Closed)
|
Created:
9 years, 8 months ago by lipalani1 Modified:
9 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionsync: Make nudge + config jobs reliable in SyncerThread2
We save nudges that are received during config. And execute them when mode changes to normal.
We also save nudge and config jobs that fail due to any reason. Then we execute them either during exponential backoff or when we get connection.
In short we are making the nudges and configs reliable.
BUG=77820
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81701
Patch Set 1 #Patch Set 2 : Fixing lint. #Patch Set 3 : Fix a bug. #
Total comments: 8
Patch Set 4 : Fixing bugs. #Patch Set 5 : Fixing a typo. #
Total comments: 14
Patch Set 6 : Fixing CR feedback. #
Total comments: 16
Patch Set 7 : Fixed CR feedback. #Patch Set 8 : Fix CR feedback and all the unittests. #
Total comments: 48
Patch Set 9 : Fixing CR feedback. #
Total comments: 30
Patch Set 10 : Fixing CR feedback. #
Total comments: 2
Patch Set 11 : Fixing a bug with polling. #
Total comments: 14
Patch Set 12 : Fixing CR feedback. #
Total comments: 30
Patch Set 13 : Fixing CR feedback. #
Total comments: 35
Patch Set 14 : Fixing CR feedback. #Patch Set 15 : Fixed a comment(it is the only change in this patch) #
Total comments: 1
Patch Set 16 : Fix sync backend host to call configure on right thread. #
Total comments: 8
Patch Set 17 : Fixing CR feedback. #Patch Set 18 : Removing the test flag. #Patch Set 19 : upload before commit. #Messages
Total messages: 41 (0 generated)
I still have to test it. Instead of going through the diffs it might make sense to read the file linearly for the new functions. I will write unit tests tomorrow.
Fixed a bug. This is the latest.
I think this approach is good, but I'm OCD about trying to keep the code more compact in this file; I want to avoid little decisions being made all over (as well as session creation, since conceptually it should only happen in a couple places!). So I have a few comments. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:191: SyncerThread::JobProcessDecision SyncerThread::ShouldRunConfigureJob() { wouldn't it result in a substantial amount less code to have these in one method as before, due to similarities? there appears to be a fair bit of duplication both among and within the split-up methods. it was easier on the mind to see it all in one place too. I think one of the main improvements of the new impl was that there was one and only one place to go look to see the rules for running / dropping jobs... we should try and preserve that. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:235: bool SyncerThread::ProcessJob(const SyncSessionJob& job) { It's pretty confusing that we now have both Process and ShouldRun... in every place that we now call processJob, it would be far more readable to call ShouldRun. e.g. while(ShouldRunJob has way less cognitive overhead than while(ProcessJob It also causes a lot of extra / duplicated code to create jobs in multiple locations. I tried quite hard to keep the job creation routine limited to ScheduleSyncSessionJob (params). That's why that method exists. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:362: if (!ProcessJob(job)) { We only had to move this down because we must pass in a job, right? And the job is only needed when purpose != NUDGE in ProcessJob... We should strive to limit the places where we have to create a session and a job. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:383: // TODO(lipalani) - pass the job itself to ScheduleSyncSessionJob. if we do that then there is no point to having ScheduleSyncSessionJob. It exists so that we only have to create a job in one place.
Let me know how this sounds... http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:191: SyncerThread::JobProcessDecision SyncerThread::ShouldRunConfigureJob() { The original function did its job well in terms of code reuse and readability. Now the new logic is little more dense. This arrangement uncomplicates the logic wrt to future changes. For ex: If I make a chance in Nudge it wont cause a regression in configure or default. Currently the only common code is checking for throttling(in nudge and configure. Even default does not share it. Interestingly the common code most of the time is shared between only 2 modes and the third is different!). Even in future if we want a certain condition to have common behavior across all the modes it makes to think of them seperately but write a common function which all the modes call.(Currently I dont see such a refactor possible. But if you see one let me know) On 2011/04/07 06:13:07, timsteele wrote: > wouldn't it result in a substantial amount less code to have these in one method > as before, due to similarities? there appears to be a fair bit of duplication > both among and within the split-up methods. it was easier on the mind to see it > all in one place too. I think one of the main improvements of the new impl was > that there was one and only one place to go look to see the rules for running / > dropping jobs... we should try and preserve that. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:235: bool SyncerThread::ProcessJob(const SyncSessionJob& job) { Will do. Regarding job creation it was also done in HandleConsecutiveError case. This code is similar to that.. Let me know what you think. The other option I see is just storing the datatypes. So job creation still lies with schedulesyncsessionjob. I am not passionate about one way or the other. The reason I picked job was it had all the data like purpose and modeltypes in one structure. If we passed in model types then we have to store the purpose as well seperately and also the source. On 2011/04/07 06:13:07, timsteele wrote: > It's pretty confusing that we now have both Process and ShouldRun... > in every place that we now call processJob, it would be far more readable to > call ShouldRun. e.g. while(ShouldRunJob has way less cognitive overhead than > while(ProcessJob > > It also causes a lot of extra / duplicated code to create jobs in multiple > locations. I tried quite hard to keep the job creation routine limited to > ScheduleSyncSessionJob (params). That's why that method exists. > > > http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:362: if (!ProcessJob(job)) { The other option as I mentioned was to pass only the datatypes. On 2011/04/07 06:13:07, timsteele wrote: > We only had to move this down because we must pass in a job, right? > And the job is only needed when purpose != NUDGE in ProcessJob... > We should strive to limit the places where we have to create a session and a > job. http://codereview.chromium.org/6812004/diff/4001/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:383: // TODO(lipalani) - pass the job itself to ScheduleSyncSessionJob. Same as above. On 2011/04/07 06:13:07, timsteele wrote: > if we do that then there is no point to having ScheduleSyncSessionJob. > It exists so that we only have to create a job in one place.
As we discussed, consider a canary job type or bool so that the subtlety of
had-nudge and IsRunning is more explicit.
Also, I played around with this locally and while I see your point about
avoiding per-job-purpose regressions, I remain convinced (I convinced myself
when writing the initial patch too!) that it is better if we split it along the
responsibilities of ShouldRunJob, namely "wait interval", "mode" and
"freshness". For example, much of the function would reduce to:
SyncerThread::JobProcessDecision SyncerThread::ShouldRunJob(
SyncSessionJobPurpose purpose, const TimeTicks& scheduled_start) {
if (purpose == CLEAR_USER_DATA)
return CONTINUE;
if (wait_interval_.get()) {
if (purpose == POLL)
return DROP;
DCHECK(purpose == NUDGE || purpose == CONFIGURATION);
if (wait_interval_->mode == WaitInterval::THROTTLED)
return SAVE;
DCHECK_EQ(wait_interval_->mode, WaitInterval::EXPONENTIAL_BACKOFF);
if (purpose == NUDGE) {
return mode_ == CONFIGURATION_MODE ? SAVE :
wait_interval_->had_nudge ? DROP : CONTINUE;
}
if (purpose == CONFIGURATION)
return wait_interval_->timer.IsRunning() ? SAVE : CONTINUE;
}
}
I find this to be less tangled and easier to reason with.
And note that A) you could still factor out a CheckWaitInterval CheckMode etc
function if you wanted to, B) having a CANARY mode or bool may simplify this
further, C) I'm not sure we want to save a nudge if in backoff when in config
mode?
WDYT
this actually this looks pretty good. Does not have any of the messiness I was worried about! I will change it and upload it to the next patch along with bug fixes in an hour or so. On Thu, Apr 7, 2011 at 1:52 PM, <tim@chromium.org> wrote: > As we discussed, consider a canary job type or bool so that the subtlety of > had-nudge and IsRunning is more explicit. > > Also, I played around with this locally and while I see your point about > avoiding per-job-purpose regressions, I remain convinced (I convinced > myself > when writing the initial patch too!) that it is better if we split it along > the > responsibilities of ShouldRunJob, namely "wait interval", "mode" and > "freshness". For example, much of the function would reduce to: > > SyncerThread::JobProcessDecision SyncerThread::ShouldRunJob( > SyncSessionJobPurpose purpose, const TimeTicks& scheduled_start) { > if (purpose == CLEAR_USER_DATA) > return CONTINUE; > > if (wait_interval_.get()) { > if (purpose == POLL) > return DROP; > > DCHECK(purpose == NUDGE || purpose == CONFIGURATION); > if (wait_interval_->mode == WaitInterval::THROTTLED) > return SAVE; > > DCHECK_EQ(wait_interval_->mode, WaitInterval::EXPONENTIAL_BACKOFF); > if (purpose == NUDGE) { > return mode_ == CONFIGURATION_MODE ? SAVE : > wait_interval_->had_nudge ? DROP : CONTINUE; > } > > if (purpose == CONFIGURATION) > return wait_interval_->timer.IsRunning() ? SAVE : CONTINUE; > } > > } > > I find this to be less tangled and easier to reason with. > > And note that A) you could still factor out a CheckWaitInterval CheckMode > etc > function if you wanted to, B) having a CANARY mode or bool may simplify > this > further, C) I'm not sure we want to save a nudge if in backoff when in > config > mode? > > WDYT > > > http://codereview.chromium.org/6812004/ >
Fixed couple of bugs and added CR feedback. In particular: 1. A nudge is never saved inside wait_interval_(only config jobs are saved there. Earlier both could be saved.) instead it is saved by setting the boolean saved_nudge only. Saves us some unnecessary logic in not having to check for it in 2 places. 2. The nudge source is saved as saved_source_ so when we reissue it we can pass the right source. 3. Refactored the shouldrunjob method.
Still havent cleaned up the ScheduleSyncSessionJob to make sure jobs are created in one place or at least getting rid of the function. Plan to do it in a future review. Currently left it as a todo. But if you are concerned dont mind fixing it in this review.
Fixed a typo!! I still have to write unit tests. I plan to do it in this patch itself.
Biggest change here: 1. got rid of the bool variables and moved to the original design of having 2 stored jobs. It was a good experience to iterate on the design to prove that this is optimal.(well we might still iterate and find a more optimal one as well!!) 2. fixed so that all current unit tests pass.(one unit test is slightly rewritten. others pass as such)
had a few comments on an older patchset that still apply. i'll look at the latest one tomorrow. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:205: // If our timer ran out then continue. this can go on above line. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:214: SyncerThread::JobProcessDecision SyncerThread::ProcessJob( Perhaps MakeJobDecision or DecideOnJob Process is misleading. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:216: if (job.purpose == CLEAR_USER_DATA) { this file in general follows the "no { } on single line if statement", and the style position is to follow convention as no style is required. So you should audit your ifs for this. (Or change the rest of the file technically but please don't do that :) ) http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:221: return DecideOnJobWhileBackingOff(job); this method could be named better e.g. not 'BackingfOff' but InWaitInterval http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:252: if (decision == DROP) { I'd collapse these 2 if statements into if (decision != SAVE) return decision == CONTINUE; then the DCHECK_EQ below isn't even needed as it's implicit. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:267: DCHECK(mode_ == CONFIGURATION_MODE); how do we know this?? http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:358: session = NULL; what's this for?
Hey Lingesh, please address the comments here and from last night. I'll take a look after! thanks. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:270: bool saved_nudge_; think you meant to remove these two fields.
I will send the next CR with the unit tests. However let me know if you agree with some of the feedback I have given below. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:205: // If our timer ran out then continue. On 2011/04/08 03:37:46, timsteele wrote: > this can go on above line. Done. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:214: SyncerThread::JobProcessDecision SyncerThread::ProcessJob( On 2011/04/08 03:37:46, timsteele wrote: > Perhaps MakeJobDecision or DecideOnJob > Process is misleading. Done. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:216: if (job.purpose == CLEAR_USER_DATA) { Left as such if the if had an else clause. Otherwise changed it. On 2011/04/08 03:37:46, timsteele wrote: > this file in general follows the "no { } on single line if statement", and the > style position is to follow convention as no style is required. So you should > audit your ifs for this. (Or change the rest of the file technically but please > don't do that :) ) Done. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:221: return DecideOnJobWhileBackingOff(job); On 2011/04/08 03:37:46, timsteele wrote: > this method could be named better e.g. not 'BackingfOff' but InWaitInterval Done. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:252: if (decision == DROP) { I am OK with changing it. But readability wise this looks more readable. And also lesser regressions. case in point: if we introduce a 4th state then the above line has to change. However let me know. On 2011/04/08 03:37:46, timsteele wrote: > I'd collapse these 2 if statements into > > if (decision != SAVE) > return decision == CONTINUE; > > then the DCHECK_EQ below isn't even needed as it's implicit. http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:267: DCHECK(mode_ == CONFIGURATION_MODE); If the purpose is config then we should be in config mode. otherwise it is a bug. On 2011/04/08 03:37:46, timsteele wrote: > how do we know this?? http://codereview.chromium.org/6812004/diff/3004/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:358: session = NULL; So that nobody reuses session. I can put it in a scope if that makes it easier. On 2011/04/08 03:37:46, timsteele wrote: > what's this for? http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:270: bool saved_nudge_; On 2011/04/08 18:08:04, timsteele wrote: > think you meant to remove these two fields. > Done.
http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:219: // let us retry once more. if the timer ran out.. is that still valid now that we have is_canary? http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:225: if (job.is_canary_job) { I want to use ternary form here. e.g. return is_canary_job ? CONTINUE : SAVE I think it's healthier overall for the this file if this function / group of functions deciding on jobs is as crisp and succinct as it can be right now. I'd argue this for the above too, since we could collapse 11 lines into three (see my earlier code review response). And put the comments together, above the code. It's a bit intimidating to read this function as it's sprawling out... http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:269: void SyncerThread::CoalescePendingNudge(const SyncSessionJob& job) { i still see places in the code where Coalesce is called directly. this also does more than coalesce, it actually initializes pending_nudge_. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:297: return true; I still think we should combine these ifs as I suggested earlier. There is no 4th state. I want to keep this as crisp as possible. This function becomes just if (decision != SAVE) return decision == CONTINUE else SaveJob() which is close to being inline-able http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:697: // or clear user data) we are going to treat text formatting not proper here. also why can't it be clear user data Why do we want to coalesce now whereas before we'd always replace? http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:735: wait_interval_->had_nudge = false; this should not be required anymore. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:751: void SyncerThread::ScheduleCanaryJob(SyncSessionJob* job) { this function is exactly what DoCanaryJob did before so I think that name is better suited than Schedule. Seems like it'd be cleaner to get rid of the two functions and just keep one called DoCanaryJob. It would have the following: SyncSessionJob* job = NULL; if (mode_ == CONFIGURATION_MODE && wait_interval_.get() && wait_interval_->pending_configure_job.get()) { job = wait_interval_->pending_configure_job.get(); } else if (pending_nudge_.get()) { job = pending_nudge_.get(); } if (!job) return; SyncSessionJob copy = *job; copy.is_canary_job = true; DoSyncSessionJob(copy);
Fixed CR feedback. Added unit tests for DecideOnJob function. Now working on adding integration tests for end to end tests. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:219: // let us retry once more. On 2011/04/08 20:57:41, timsteele wrote: > if the timer ran out.. is that still valid now that we have is_canary? Done. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:225: if (job.is_canary_job) { On 2011/04/08 20:57:41, timsteele wrote: > I want to use ternary form here. > e.g. return is_canary_job ? CONTINUE : SAVE > > I think it's healthier overall for the this file if this function / group of > functions deciding on jobs is as crisp and succinct as it can be right now. > > I'd argue this for the above too, since we could collapse 11 lines into three > (see my earlier code review response). And put the comments together, above the > code. It's a bit intimidating to read this function as it's sprawling out... Done. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:269: void SyncerThread::CoalescePendingNudge(const SyncSessionJob& job) { Renamed it to updatependingjob. On 2011/04/08 20:57:41, timsteele wrote: > i still see places in the code where Coalesce is called directly. > > this also does more than coalesce, it actually initializes pending_nudge_. Done. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:297: return true; On 2011/04/08 20:57:41, timsteele wrote: > I still think we should combine these ifs as I suggested earlier. There is no > 4th state. I want to keep this as crisp as possible. > > This function becomes just > > if (decision != SAVE) > return decision == CONTINUE > else > SaveJob() > > which is close to being inline-able Done. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:697: // or clear user data) we are going to treat added a todo for clearuserdata. On 2011/04/08 20:57:41, timsteele wrote: > text formatting not proper here. > also why can't it be clear user data > > Why do we want to coalesce now whereas before we'd always replace? Done. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:735: wait_interval_->had_nudge = false; This is the only place it gets cleared out. We want the semantics of allowing one nudge per backoff interval. And it gets cleared when the backoff interval runs out which is here. On 2011/04/08 20:57:41, timsteele wrote: > this should not be required anymore. http://codereview.chromium.org/6812004/diff/4005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:751: void SyncerThread::ScheduleCanaryJob(SyncSessionJob* job) { Sorry this was part of the unfinished refactoring I mentioned. Actually this function is needed by took your feedback and renamed it to ExecuteJobByMakingACopy(hope the name is ok). This is called from few more places(like when switching from config mode to normal mode etc). look at the latest review. On 2011/04/08 20:57:41, timsteele wrote: > this function is exactly what DoCanaryJob did before so I think that name is > better suited than Schedule. Seems like it'd be cleaner to get rid of the two > functions and just keep one called DoCanaryJob. > It would have the following: > > SyncSessionJob* job = NULL; > if (mode_ == CONFIGURATION_MODE && wait_interval_.get() && > wait_interval_->pending_configure_job.get()) { > job = wait_interval_->pending_configure_job.get(); > } else if (pending_nudge_.get()) { > job = pending_nudge_.get(); > } > > if (!job) > return; > > SyncSessionJob copy = *job; > copy.is_canary_job = true; > DoSyncSessionJob(copy);
OK this patch has all the functionalities and all the unit tests and has CR feedback incorporated upto now.
please bear with me here! http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... File chrome/browser/sync/engine/mock_model_safe_workers.h (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... chrome/browser/sync/engine/mock_model_safe_workers.h:33: static MockModelSafeWorkerRegistrar* SetPassiveTypes( s/SetPassiveTypes/WithPassiveTypes or FromPassiveTypes or ForPassiveTypes http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:131: ExecutePendingJob(); this kind of looks out of place. at the very least it needs a comment. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:138: DCHECK(job.purpose != CLEAR_USER_DATA); DCHECK_NE http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:156: // This is a config job. If our timer ran out then continue else save. Saying it's a config job is fine but I'd avoid referencing the timer. I think the line of code is self commenting in that regard. Also, you should remove the 'else' clause. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:170: if (job.purpose == NUDGE) { remove the { }s from this inner if as they're 1-liners. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:180: DCHECK(mode_ == NORMAL_MODE); DCHECK_EQ http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:181: DCHECK(job.purpose != CONFIGURATION); I don't think this should be a dcheck. The mode changes asynchronously from job requests, so nothing prevents a config task from trying to run in normal mode. The previous ShouldRunJob function was designed to support this and just drop. There should have been tests that tried this :\ So I don't think we should assert. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:193: void SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { I've looked at the callsites of this function and it leaves me kind of confused as to its purpose; it seems each callsite is after precisely one of the two cases in this method? It might honestly be more clear to inline it instead. I realize this may have been an attempt to reduce the places we create a SyncSessionJob. Or what I might do is take the if (!pending_nudge_.get()) case and put that in a method called CreatePendingJob, but remove the coalescing part altogether and inline that. It's helpful to be able to search for 'Coalesce' and immediately find the places where that is performed. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:217: return decision == CONTINUE ? true : false; remove the ternary here, it is redundant to decision==CONTINUE. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == NUDGE || job.purpose == POLL) { We should never want to save a POLL. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:329: UpdatePendingJob(job); I think this was more clear when we just did the call to Coalesce inline. Update just performs the same check anyway. I'd prefer if we moved it back. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:472: SaveJob(job); This if-block seems out of place...we should be doing as little as possible. I think if anything this should be moved to the IsSyncingCurrentlySilenced if-block inside FinishSyncSessionJob; we shouldn't let that logic / decision making creep out if possible. Also add a comment there why it is needed... we have to be careful because a job could be partially done if we managed to call SyncShare. Lastly though, the DCHECK is not needed because SaveJob does this. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:609: // We are not in configuration mode. So wait_interval's pending job I'm rather concerned about the apparent connection this CL makes between job.purpose == CONFIGURATION and mode_ == CONFIGURATION :\ That is not the design. They are orthogonal constructs so that they are *not* coupled to each other. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:56: enum SyncSessionJobPurpose { This was originally in the header file, and was called SyncSessionJob::Purpose. It was then pulled out of the struct only so that the struct could be moved to the cc file for clang (http://codereview.chromium.org/6272025), but now you've brought that back. I'll let you look at that review and figure out what to do, but if they are both to be in the header file, this should be declared inside SyncSessionJob as 'Purpose'. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:215: JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job); the functions in this file are roughly ordered in dependency order, e.g. if DecideOnJob calls DecideWhileInWaitInterval, please order accordingly. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:246: void ExecutePendingJob(); How is execute different from 'DoSyncSessionJob'. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:248: void ExecuteJobByMakingACopy(SyncSessionJob* job); These two functions together are confusing, especially when there's a DoSyncSessionJob. I think there should only be two functions: DoSyncSessionJob(job) and DoPendingJobIfPossible() the 'ByMakingACopy' variant I think adds more questions than it's worth; i'd inline it. You could probably do SyncSessionJob job; job.is_canary = true; if CONFIG ... else ... instead, in DoPendingJob. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: syncer_thread_->mode_ = mode; it seems bad / dangerous to have all these setters here. The tests in this file should be doing it the normal, non-racy way by using the public API. These tests are not intended to be as white-box as these methods allow. If these functions are only needed for a subset of the tests (e.g. the ones testing decidejob), then at the very least they should be part of a separate test fixture, or else I guarantee one day someone will try and use these in the normal tests. I'd almost say we should move the new ones to a separate file. But at a minimu, we need a separate test fixture e.g. class SyncerThread2JobDecisionsTest { { http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:189: void SetWaitIntervalToThrottled() { the existing tests would set this state naturally with test_util. It's confusing to have both approaches used. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:303: EXPECT_CALL(*syncer(), SyncShare(_,_,_)) In general it's bad to have the expectations before the call to Start as google mock is not threadsafe. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:369: how come all these tests now wait after each post... seems rather simulated http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:377: TEST_F(SyncerThread2Test, NudgeWithConfigWithBackingOff) { it's not clear what is being tested here versus BackoffDropsJobs. Note the existing tests had comments explaining what was going on. Please do the same, and try to contrast the differences. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:679: FlushLastTask(&done); Why are there calls to this sprinkled over this test? They appear to serialize all requests and this was trying to test multiple at the same time.
Hi thanks this is great. In retrospect I should have moved the new type of unit tests to a new file. I will do so. I will get back to you in some more time with your comments fixed and feedback if I have any. Thanks! regards, Lingesh On Sat, Apr 9, 2011 at 4:52 PM, <tim@chromium.org> wrote: > please bear with me here! > > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... > File chrome/browser/sync/engine/mock_model_safe_workers.h (right): > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... > chrome/browser/sync/engine/mock_model_safe_workers.h:33: static > MockModelSafeWorkerRegistrar* SetPassiveTypes( > s/SetPassiveTypes/WithPassiveTypes > or FromPassiveTypes > or ForPassiveTypes > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > > File chrome/browser/sync/engine/syncer_thread2.cc (right): > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:131: ExecutePendingJob(); > this kind of looks out of place. at the very least it needs a comment. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:138: DCHECK(job.purpose != > CLEAR_USER_DATA); > DCHECK_NE > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:156: // This is a config > job. If our timer ran out then continue else save. > Saying it's a config job is fine but I'd avoid referencing the timer. I > think the line of code is self commenting in that regard. > > Also, you should remove the 'else' clause. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:170: if (job.purpose == > NUDGE) { > remove the { }s from this inner if as they're 1-liners. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:180: DCHECK(mode_ == > NORMAL_MODE); > DCHECK_EQ > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:181: DCHECK(job.purpose != > CONFIGURATION); > I don't think this should be a dcheck. The mode changes asynchronously > from job requests, so nothing prevents a config task from trying to run > in normal mode. The previous ShouldRunJob function was designed to > support this and just drop. There should have been tests that tried > this :\ So I don't think we should assert. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:193: void > SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { > I've looked at the callsites of this function and it leaves me kind of > confused as to its purpose; it seems each callsite is after precisely > one of the two cases in this method? It might honestly be more clear to > inline it instead. I realize this may have been an attempt to reduce > the places we create a SyncSessionJob. Or what I might do is take the > if (!pending_nudge_.get()) case and put that in a method called > CreatePendingJob, but remove the coalescing part altogether and inline > that. It's helpful to be able to search for 'Coalesce' and immediately > find the places where that is performed. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:217: return decision == > CONTINUE ? true : false; > remove the ternary here, it is redundant to decision==CONTINUE. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == > NUDGE || job.purpose == POLL) { > We should never want to save a POLL. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:329: UpdatePendingJob(job); > I think this was more clear when we just did the call to Coalesce > inline. Update just performs the same check anyway. I'd prefer if we > moved it back. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:472: SaveJob(job); > This if-block seems out of place...we should be doing as little as > possible. I think if anything this should be moved to the > IsSyncingCurrentlySilenced if-block inside FinishSyncSessionJob; we > shouldn't let that logic / decision making creep out if possible. Also > add a comment there why it is needed... we have to be careful because a > job could be partially done if we managed to call SyncShare. > > Lastly though, the DCHECK is not needed because SaveJob does this. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:609: // We are not in > configuration mode. So wait_interval's pending job > I'm rather concerned about the apparent connection this CL makes between > job.purpose == CONFIGURATION and mode_ == CONFIGURATION :\ That is not > the design. They are orthogonal constructs so that they are *not* > coupled to each other. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2.h (right): > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.h:56: enum > SyncSessionJobPurpose { > This was originally in the header file, and was called > SyncSessionJob::Purpose. It was then pulled out of the struct only so > that the struct could be moved to the cc file for clang > (http://codereview.chromium.org/6272025), but now you've brought that > back. I'll let you look at that review and figure out what to do, but > if they are both to be in the header file, this should be declared > inside SyncSessionJob as 'Purpose'. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.h:215: JobProcessDecision > DecideWhileInWaitInterval(const SyncSessionJob& job); > the functions in this file are roughly ordered in dependency order, e.g. > if DecideOnJob calls DecideWhileInWaitInterval, please order > accordingly. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.h:246: void > ExecutePendingJob(); > How is execute different from 'DoSyncSessionJob'. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.h:248: void > ExecuteJobByMakingACopy(SyncSessionJob* job); > These two functions together are confusing, especially when there's a > DoSyncSessionJob. I think there should only be two functions: > > DoSyncSessionJob(job) > and > DoPendingJobIfPossible() > > the 'ByMakingACopy' variant I think adds more questions than it's worth; > i'd inline it. You could probably do > > SyncSessionJob job; > job.is_canary = true; > > if CONFIG > ... > else > ... > > instead, in DoPendingJob. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: > syncer_thread_->mode_ = mode; > it seems bad / dangerous to have all these setters here. > The tests in this file should be doing it the normal, non-racy way by > using the public API. These tests are not intended to be as white-box > as these methods allow. > > If these functions are only needed for a subset of the tests (e.g. the > ones testing decidejob), then at the very least they should be part of a > separate test fixture, or else I guarantee one day someone will try and > use these in the normal tests. I'd almost say we should move the new > ones to a separate file. But at a minimu, we need a separate test > fixture > > e.g. class SyncerThread2JobDecisionsTest { { > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:189: void > SetWaitIntervalToThrottled() { > the existing tests would set this state naturally with test_util. It's > confusing to have both approaches used. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:303: > EXPECT_CALL(*syncer(), SyncShare(_,_,_)) > In general it's bad to have the expectations before the call to Start as > google mock is not threadsafe. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:369: > how come all these tests now wait after each post... seems rather > simulated > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:377: > TEST_F(SyncerThread2Test, NudgeWithConfigWithBackingOff) { > it's not clear what is being tested here versus BackoffDropsJobs. > > Note the existing tests had comments explaining what was going on. > Please do the same, and try to contrast the differences. > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:679: > FlushLastTask(&done); > Why are there calls to this sprinkled over this test? They appear to > serialize all requests and this was trying to test multiple at the same > time. > > > http://codereview.chromium.org/6812004/ >
Fixing CR feedback. Both integration and unit tests pass. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... File chrome/browser/sync/engine/mock_model_safe_workers.h (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/m... chrome/browser/sync/engine/mock_model_safe_workers.h:33: static MockModelSafeWorkerRegistrar* SetPassiveTypes( On 2011/04/09 23:52:58, timsteele wrote: > s/SetPassiveTypes/WithPassiveTypes > or FromPassiveTypes > or ForPassiveTypes Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:131: ExecutePendingJob(); On 2011/04/09 23:52:58, timsteele wrote: > this kind of looks out of place. at the very least it needs a comment. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:138: DCHECK(job.purpose != CLEAR_USER_DATA); On 2011/04/09 23:52:58, timsteele wrote: > DCHECK_NE Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:156: // This is a config job. If our timer ran out then continue else save. On 2011/04/09 23:52:58, timsteele wrote: > Saying it's a config job is fine but I'd avoid referencing the timer. I think > the line of code is self commenting in that regard. > > Also, you should remove the 'else' clause. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:170: if (job.purpose == NUDGE) { On 2011/04/09 23:52:58, timsteele wrote: > remove the { }s from this inner if as they're 1-liners. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:180: DCHECK(mode_ == NORMAL_MODE); On 2011/04/09 23:52:58, timsteele wrote: > DCHECK_EQ Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:181: DCHECK(job.purpose != CONFIGURATION); Not sure I follow this. Let me talk to you today when I get in. On 2011/04/09 23:52:58, timsteele wrote: > I don't think this should be a dcheck. The mode changes asynchronously from job > requests, so nothing prevents a config task from trying to run in normal mode. > The previous ShouldRunJob function was designed to support this and just drop. > There should have been tests that tried this :\ So I don't think we should > assert. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:193: void SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { There are 3 callsites. 2 of them(from savejob and handleconsecutiveerror could get into both the if or else part. The one from ScheudleNudgeImpl can only get into the else part. I can move that inline. On 2011/04/09 23:52:58, timsteele wrote: > I've looked at the callsites of this function and it leaves me kind of confused > as to its purpose; it seems each callsite is after precisely one of the two > cases in this method? It might honestly be more clear to inline it instead. I > realize this may have been an attempt to reduce the places we create a > SyncSessionJob. Or what I might do is take the if (!pending_nudge_.get()) case > and put that in a method called CreatePendingJob, but remove the coalescing part > altogether and inline that. It's helpful to be able to search for 'Coalesce' > and immediately find the places where that is performed. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:217: return decision == CONTINUE ? true : false; ha ha ! may be you got confused as I mentioned!!(or may be I made a mistake). Both are needed. There are 3 choices: save, drop and continue. if not save and continue then return true . if not save and not continue return false. On 2011/04/09 23:52:58, timsteele wrote: > remove the ternary here, it is redundant to decision==CONTINUE. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == NUDGE || job.purpose == POLL) { The case here is when in config but a poll comes in. On 2011/04/09 23:52:58, timsteele wrote: > We should never want to save a POLL. > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:329: UpdatePendingJob(job); On 2011/04/09 23:52:58, timsteele wrote: > I think this was more clear when we just did the call to Coalesce inline. > Update just performs the same check anyway. I'd prefer if we moved it back. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:472: SaveJob(job); Good point. Done. On 2011/04/09 23:52:58, timsteele wrote: > This if-block seems out of place...we should be doing as little as possible. I > think if anything this should be moved to the IsSyncingCurrentlySilenced > if-block inside FinishSyncSessionJob; we shouldn't let that logic / decision > making creep out if possible. Also add a comment there why it is needed... we > have to be careful because a job could be partially done if we managed to call > SyncShare. > > Lastly though, the DCHECK is not needed because SaveJob does this. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:609: // We are not in configuration mode. So wait_interval's pending job Hmm.. not sure I follow. let me sync up with you. On 2011/04/09 23:52:58, timsteele wrote: > I'm rather concerned about the apparent connection this CL makes between > job.purpose == CONFIGURATION and mode_ == CONFIGURATION :\ That is not the > design. They are orthogonal constructs so that they are *not* coupled to each > other. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:56: enum SyncSessionJobPurpose { On 2011/04/09 23:52:58, timsteele wrote: > This was originally in the header file, and was called SyncSessionJob::Purpose. > It was then pulled out of the struct only so that the struct could be moved to > the cc file for clang (http://codereview.chromium.org/6272025), but now you've > brought that back. I'll let you look at that review and figure out what to do, > but if they are both to be in the header file, this should be declared inside > SyncSessionJob as 'Purpose'. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:215: JobProcessDecision DecideWhileInWaitInterval(const SyncSessionJob& job); On 2011/04/09 23:52:58, timsteele wrote: > the functions in this file are roughly ordered in dependency order, e.g. if > DecideOnJob calls DecideWhileInWaitInterval, please order accordingly. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:246: void ExecutePendingJob(); On 2011/04/09 23:52:58, timsteele wrote: > How is execute different from 'DoSyncSessionJob'. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.h:248: void ExecuteJobByMakingACopy(SyncSessionJob* job); On 2011/04/09 23:52:58, timsteele wrote: > These two functions together are confusing, especially when there's a > DoSyncSessionJob. I think there should only be two functions: > > DoSyncSessionJob(job) > and > DoPendingJobIfPossible() > > the 'ByMakingACopy' variant I think adds more questions than it's worth; i'd > inline it. You could probably do > > SyncSessionJob job; > job.is_canary = true; > > if CONFIG > ... > else > ... > > instead, in DoPendingJob. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: syncer_thread_->mode_ = mode; Moved to a different file. On 2011/04/09 23:52:58, timsteele wrote: > it seems bad / dangerous to have all these setters here. > The tests in this file should be doing it the normal, non-racy way by using the > public API. These tests are not intended to be as white-box as these methods > allow. > > If these functions are only needed for a subset of the tests (e.g. the ones > testing decidejob), then at the very least they should be part of a separate > test fixture, or else I guarantee one day someone will try and use these in the > normal tests. I'd almost say we should move the new ones to a separate file. > But at a minimu, we need a separate test fixture > > e.g. class SyncerThread2JobDecisionsTest { { Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:189: void SetWaitIntervalToThrottled() { This is moved to a different file. It is needed there for white box testing. On 2011/04/09 23:52:58, timsteele wrote: > the existing tests would set this state naturally with test_util. It's > confusing to have both approaches used. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:303: EXPECT_CALL(*syncer(), SyncShare(_,_,_)) Good point. Even if they were thread safe expectations need to be set before we call start. On 2011/04/09 23:52:58, timsteele wrote: > In general it's bad to have the expectations before the call to Start as google > mock is not threadsafe. Done. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:369: Comments added explaining the rationale for them. On 2011/04/09 23:52:58, timsteele wrote: > how come all these tests now wait after each post... seems rather simulated http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:377: TEST_F(SyncerThread2Test, NudgeWithConfigWithBackingOff) { I have added comments. In short backoffdropjobs test backing off of a nudge(in normal mode) whereas this puts config in backing off state and induces a nudge. On 2011/04/09 23:52:58, timsteele wrote: > it's not clear what is being tested here versus BackoffDropsJobs. > > Note the existing tests had comments explaining what was going on. Please do > the same, and try to contrast the differences. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:679: FlushLastTask(&done); argh. failed to remove something I was playing with. On 2011/04/09 23:52:58, timsteele wrote: > Why are there calls to this sprinkled over this test? They appear to serialize > all requests and this was trying to test multiple at the same time.
Getting there, starting to look much better! http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == NUDGE || job.purpose == POLL) { On 2011/04/12 02:33:00, lipalani1 wrote: > The case here is when in config but a poll comes in. > On 2011/04/09 23:52:58, timsteele wrote: > > We should never want to save a POLL. > > > In that case I claim we want to drop the poll. We shouldn't do any work at all to make them reliable; they are a backup. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... File chrome/browser/sync/engine/mock_model_safe_workers.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... chrome/browser/sync/engine/mock_model_safe_workers.cc:27: MockModelSafeWorkerRegistrar* MockModelSafeWorkerRegistrar::FromPassiveTypes( hm okay, so looking at this I think 'PassiveForTypes' is more adequate. The types aren't passive, but we want to make a fully 'passive' routing info for the types passed in. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:235: return decision == CONTINUE ? true : false; what I meant before is that this is equivalent to "return decision == CONTINUE" which is easier to read :) http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:248: UpdatePendingJob(job); What I was saying before was this might be more readable as if (pending_nudge_.get()) pending_nudge_->Coalesce else InitializePendingJob and remove the Coalesce case from (and rename) UpdatePendingJob http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:614: DCHECK(!IsBackingOff() || wait_interval_.get() != NULL); this appears to be the opposite condition from before... why the change? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:685: if (wait_interval_.get() && wait_interval_->pending_configure_job.get()) { combine this with the above if statement... less nesting is easier on the brain! http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:689: if (pending_nudge_.get()) { move this into 'else if' above. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:713: SyncSession* SyncerThread::CreateSyncSession(const SyncSourceInfo& source) { I think this could be used in 4 (?) more places if it also took in an 'old' session param to take routing info from. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:735: SyncSession* s = new SyncSession(session_context_.get(), this, info, r, w); can this use CreateSyncSession? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:77: registrar_.reset(MockModelSafeWorkerRegistrar::SetPassiveTypes(types)); SetPassiveTypes? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: scoped_ptr<SyncerThread> syncer_thread_; this change isn't necessary? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:271: TEST_F(SyncerThread2Test, ConfigWithBackingOff) { still missing comments. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:296: EXPECT_EQ(GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION, what about snapshots [0]? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:905: EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done)); hmm... if the server is not reachable, we shouldn't get this. this seems to be the opposite of before. was this just an experiment? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:24: class SyncerThread2WhiteboxUnitTest : public testing::Test { convention in chrome is to just suffix the class with 'Test', matching the testing::Test superclass (instead of 'UnitTest') http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:109: DCHECK_EQ(decision, SyncerThread::SAVE); Tests uses 'EXPECT_EQ' or ASSERT_EQ, not dcheck (think you want expect here).
Fixed CR feedback. Currently doing manual testing with new profile, old profile, enabling/disabling types and making sure the right breakpoints are hit. http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == NUDGE || job.purpose == POLL) { Actually the code was already dropping it!! On 2011/04/12 06:09:29, timsteele wrote: > On 2011/04/12 02:33:00, lipalani1 wrote: > > The case here is when in config but a poll comes in. > > On 2011/04/09 23:52:58, timsteele wrote: > > > We should never want to save a POLL. > > > > > > > In that case I claim we want to drop the poll. We shouldn't do any work at all > to make them reliable; they are a backup. Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... File chrome/browser/sync/engine/mock_model_safe_workers.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... chrome/browser/sync/engine/mock_model_safe_workers.cc:27: MockModelSafeWorkerRegistrar* MockModelSafeWorkerRegistrar::FromPassiveTypes( On 2011/04/12 06:09:29, timsteele wrote: > hm > okay, so looking at this I think 'PassiveForTypes' is more adequate. The types > aren't passive, but we want to make a fully 'passive' routing info for the types > passed in. > Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:235: return decision == CONTINUE ? true : false; On 2011/04/12 06:09:29, timsteele wrote: > what I meant before is that this is equivalent to "return decision == CONTINUE" > which is easier to read :) Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:248: UpdatePendingJob(job); Hmm.. this is used in 2 places. SaveJob and HandleConsecutiveError. Should I rename the function? On 2011/04/12 06:09:29, timsteele wrote: > What I was saying before was this might be more readable as > if (pending_nudge_.get()) > pending_nudge_->Coalesce > else > InitializePendingJob > > and remove the Coalesce case from (and rename) UpdatePendingJob http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:614: DCHECK(!IsBackingOff() || wait_interval_.get() != NULL); What we want to check is if wait interval pointer is not null if we are bcking off.(actually it is not that big of a deal as IsBackingOff just checks the presence of the pointer!!) I think the original code wanted to check if we are backing off we must be running the timer. But that is not entirely true if we are backing off and post a canary job and that fails. When that fails backing off will be true but timer running will be false. I am rewriting this DCHECK to say if we are backing off either the timer is running or it is a canary job. On 2011/04/12 06:09:29, timsteele wrote: > this appears to be the opposite condition from before... why the change? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:685: if (wait_interval_.get() && wait_interval_->pending_configure_job.get()) { On 2011/04/12 06:09:29, timsteele wrote: > combine this with the above if statement... less nesting is easier on the brain! Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:689: if (pending_nudge_.get()) { On 2011/04/12 06:09:29, timsteele wrote: > move this into 'else if' above. Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:713: SyncSession* SyncerThread::CreateSyncSession(const SyncSourceInfo& source) { I have added one more place(polltimer) where this function is called from. However adding an old job would complicate this method. I think the right solution there is session should have a clone method which returns another session. (However I am not adding that part of this CL. However if you want it we can do that as well). On 2011/04/12 06:09:29, timsteele wrote: > I think this could be used in 4 (?) more places if it also took in an 'old' > session param to take routing info from. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2.cc:735: SyncSession* s = new SyncSession(session_context_.get(), this, info, r, w); On 2011/04/12 06:09:29, timsteele wrote: > can this use CreateSyncSession? Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:77: registrar_.reset(MockModelSafeWorkerRegistrar::SetPassiveTypes(types)); On 2011/04/12 06:09:29, timsteele wrote: > SetPassiveTypes? Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: scoped_ptr<SyncerThread> syncer_thread_; On 2011/04/12 06:09:29, timsteele wrote: > this change isn't necessary? Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:271: TEST_F(SyncerThread2Test, ConfigWithBackingOff) { On 2011/04/12 06:09:29, timsteele wrote: > still missing comments. Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:296: EXPECT_EQ(GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION, I added the comments inside the TC. Moved them up now. Agree it is easier to find what the TC is doing that way. On 2011/04/12 06:09:29, timsteele wrote: > what about snapshots [0]? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_unittest.cc:905: EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done)); I think the intention of the old and new models are the same. When the server connection comes back the already posted nudge will get executed. On 2011/04/12 06:09:29, timsteele wrote: > hmm... if the server is not reachable, we shouldn't get this. this seems to be > the opposite of before. was this just an experiment? http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... File chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc (right): http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:24: class SyncerThread2WhiteboxUnitTest : public testing::Test { On 2011/04/12 06:09:29, timsteele wrote: > convention in chrome is to just suffix the class with 'Test', matching the > testing::Test superclass (instead of 'UnitTest') Done. http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:109: DCHECK_EQ(decision, SyncerThread::SAVE); Stupid mistake. Corrected. On 2011/04/12 06:09:29, timsteele wrote: > Tests uses 'EXPECT_EQ' or ASSERT_EQ, not dcheck (think you want expect here).
actually just found a small issue. ignore this. another CR to follow. On Tue, Apr 12, 2011 at 5:07 PM, <lipalani@chromium.org> wrote: > Fixed CR feedback. > > Currently doing manual testing with new profile, old profile, > enabling/disabling > types and making sure the right breakpoints are hit. > > > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2.cc (right): > > > http://codereview.chromium.org/6812004/diff/1011/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:227: if (job.purpose == > NUDGE || job.purpose == POLL) { > Actually the code was already dropping it!! > > On 2011/04/12 06:09:29, timsteele wrote: > >> On 2011/04/12 02:33:00, lipalani1 wrote: >> > The case here is when in config but a poll comes in. >> > On 2011/04/09 23:52:58, timsteele wrote: >> > > We should never want to save a POLL. >> > > >> > >> > > In that case I claim we want to drop the poll. We shouldn't do any >> > work at all > >> to make them reliable; they are a backup. >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... > File chrome/browser/sync/engine/mock_model_safe_workers.cc (right): > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/m... > chrome/browser/sync/engine/mock_model_safe_workers.cc:27: > MockModelSafeWorkerRegistrar* > MockModelSafeWorkerRegistrar::FromPassiveTypes( > On 2011/04/12 06:09:29, timsteele wrote: > >> hm >> okay, so looking at this I think 'PassiveForTypes' is more adequate. >> > The types > >> aren't passive, but we want to make a fully 'passive' routing info for >> > the types > >> passed in. >> > > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2.cc (right): > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:235: return decision == > CONTINUE ? true : false; > On 2011/04/12 06:09:29, timsteele wrote: > >> what I meant before is that this is equivalent to "return decision == >> > CONTINUE" > >> which is easier to read :) >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:248: UpdatePendingJob(job); > Hmm.. this is used in 2 places. SaveJob and HandleConsecutiveError. > Should I rename the function? > > On 2011/04/12 06:09:29, timsteele wrote: > >> What I was saying before was this might be more readable as >> if (pending_nudge_.get()) >> pending_nudge_->Coalesce >> else >> InitializePendingJob >> > > and remove the Coalesce case from (and rename) UpdatePendingJob >> > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:614: DCHECK(!IsBackingOff() > || wait_interval_.get() != NULL); > What we want to check is if wait interval pointer is not null if we are > bcking off.(actually it is not that big of a deal as IsBackingOff just > checks the presence of the pointer!!) > I think the original code wanted to check if we are backing off we must > be running the timer. But that is not entirely true if we are backing > off and post a canary job and that fails. When that fails backing off > will be true but timer running will be false. > I am rewriting this DCHECK to say if we are backing off either the timer > is running or it is a canary job. > > On 2011/04/12 06:09:29, timsteele wrote: > >> this appears to be the opposite condition from before... why the >> > change? > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:685: if > (wait_interval_.get() && wait_interval_->pending_configure_job.get()) { > On 2011/04/12 06:09:29, timsteele wrote: > >> combine this with the above if statement... less nesting is easier on >> > the brain! > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:689: if > (pending_nudge_.get()) { > On 2011/04/12 06:09:29, timsteele wrote: > >> move this into 'else if' above. >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:713: SyncSession* > SyncerThread::CreateSyncSession(const SyncSourceInfo& source) { > I have added one more place(polltimer) where this function is called > from. However adding an old job would complicate this method. I think > the right solution there is session should have a clone method which > returns another session. (However I am not adding that part of this CL. > However if you want it we can do that as well). > > On 2011/04/12 06:09:29, timsteele wrote: > >> I think this could be used in 4 (?) more places if it also took in an >> > 'old' > >> session param to take routing info from. >> > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2.cc:735: SyncSession* s = new > SyncSession(session_context_.get(), this, info, r, w); > On 2011/04/12 06:09:29, timsteele wrote: > >> can this use CreateSyncSession? >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2_unittest.cc (right): > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:77: > registrar_.reset(MockModelSafeWorkerRegistrar::SetPassiveTypes(types)); > On 2011/04/12 06:09:29, timsteele wrote: > >> SetPassiveTypes? >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:174: > scoped_ptr<SyncerThread> syncer_thread_; > On 2011/04/12 06:09:29, timsteele wrote: > >> this change isn't necessary? >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:271: > TEST_F(SyncerThread2Test, ConfigWithBackingOff) { > On 2011/04/12 06:09:29, timsteele wrote: > >> still missing comments. >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:296: > EXPECT_EQ(GetUpdatesCallerInfo::SYNC_CYCLE_CONTINUATION, > I added the comments inside the TC. Moved them up now. Agree it is > easier to find what the TC is doing that way. > > On 2011/04/12 06:09:29, timsteele wrote: > >> what about snapshots [0]? >> > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_unittest.cc:905: > EXPECT_CALL(*syncer(), SyncShare(_,_,_)).WillOnce(SignalEvent(&done)); > I think the intention of the old and new models are the same. When the > server connection comes back the already posted nudge will get executed. > > > > On 2011/04/12 06:09:29, timsteele wrote: > >> hmm... if the server is not reachable, we shouldn't get this. this >> > seems to be > >> the opposite of before. was this just an experiment? >> > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > File chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc > (right): > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:24: class > SyncerThread2WhiteboxUnitTest : public testing::Test { > On 2011/04/12 06:09:29, timsteele wrote: > >> convention in chrome is to just suffix the class with 'Test', matching >> > the > >> testing::Test superclass (instead of 'UnitTest') >> > > Done. > > > > http://codereview.chromium.org/6812004/diff/9005/chrome/browser/sync/engine/s... > chrome/browser/sync/engine/syncer_thread2_whitebox_unittest.cc:109: > DCHECK_EQ(decision, SyncerThread::SAVE); > Stupid mistake. Corrected. > > On 2011/04/12 06:09:29, timsteele wrote: > >> Tests uses 'EXPECT_EQ' or ASSERT_EQ, not dcheck (think you want expect >> > here). > > http://codereview.chromium.org/6812004/ >
All good. Please review.
http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:225: pending_nudge_->session->Coalesce(*(job.session.get())); still here. http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:677: wait_interval_->had_nudge = false; hmm.. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:177: remove extra newline http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:211: void SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { InitOrCoalescePendingNudge http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:332: make_linked_ptr(session), is_canary_job, nit - indent http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:356: // TODO(lipalani) - pass the job itself to ScheduleSyncSessionJob. spacing http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:528: SaveJob(job); I think it'd be preferable / more explicit / readable if SaveJob just dropped POLLs. (also, style nit- remove { }'s) http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:635: // retries. - poll http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:679: // We should have one of 2 things. Otherwise we shouldnt be running the timer. as we discussed, lets move this below DoPendingJobIfPossible with a comment explaining why it is necessary, and add an as_canary param
On 2011/04/13 19:49:06, timsteele wrote: > http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... > File chrome/browser/sync/engine/syncer_thread2.cc (right): > > http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:225: > pending_nudge_->session->Coalesce(*(job.session.get())); > still here. > > http://codereview.chromium.org/6812004/diff/12001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:677: wait_interval_->had_nudge = > false; > hmm.. > > Ignore above two comments! http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:177: > remove extra newline > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:211: void > SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { > InitOrCoalescePendingNudge > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:332: make_linked_ptr(session), > is_canary_job, > nit - indent > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:356: // TODO(lipalani) - pass the > job itself to ScheduleSyncSessionJob. > spacing > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:528: SaveJob(job); > I think it'd be preferable / more explicit / readable if SaveJob just dropped > POLLs. > > (also, style nit- remove { }'s) > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:635: // retries. > - poll > > http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... > chrome/browser/sync/engine/syncer_thread2.cc:679: // We should have one of 2 > things. Otherwise we shouldnt be running the timer. > as we discussed, lets move this below DoPendingJobIfPossible with a comment > explaining why it is necessary, and add an as_canary param
Fixing CR feedback. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:177: On 2011/04/13 19:49:07, timsteele wrote: > remove extra newline Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:211: void SyncerThread::UpdatePendingJob(const SyncSessionJob& job) { On 2011/04/13 19:49:07, timsteele wrote: > InitOrCoalescePendingNudge Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:332: make_linked_ptr(session), is_canary_job, On 2011/04/13 19:49:07, timsteele wrote: > nit - indent Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:356: // TODO(lipalani) - pass the job itself to ScheduleSyncSessionJob. On 2011/04/13 19:49:07, timsteele wrote: > spacing Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:528: if (job.purpose != SyncSessionJob::POLL) { On 2011/04/13 19:49:07, timsteele wrote: > I think it'd be preferable / more explicit / readable if SaveJob just dropped > POLLs. > > (also, style nit- remove { }'s) Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:635: // No matter what type of job it is.(nudge or poll, it cannot be config) On 2011/04/13 19:49:07, timsteele wrote: > - poll Done. http://codereview.chromium.org/6812004/diff/16001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:679: wait_interval_->had_nudge = false; Fixed it by removing it altogether. As handleconsective error reinitializes this to false any way. On 2011/04/13 19:49:07, timsteele wrote: > as we discussed, lets move this below DoPendingJobIfPossible with a comment > explaining why it is necessary, and add an as_canary param
http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:101: DoPendingJobIfPossible(true); // set canary to true so this wont be dropped. Capitalize 'Set' also, see my comment @ DoCanaryJob http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:634: InitOrCoalesceJob(old_job); should include the word 'pending'Job, otherwise 'Init' is confusing :( http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:674: DoPendingJobIfPossible(true); Well, now I think we should just eliminate this method and PostTask(DoPendingJobIfPossible, true) instead of posting DoCanary case in point - because we now call DoPendingJobIfPossible(true) from the server connection event case, the DCHECK here isn't even valid because it now *is* possible to have a job with is_canary_job true but no wait interval. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:46: enum JobProcessDecision { This should not be public. Only things that the public API (which is just start/stop/schedule*) can do should be public. Also, comment above the enum http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:55: struct SyncSessionJob { this should not be public. Was this for tests? http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:89: struct WaitInterval { this should not be public. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:171: remove newline http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:215: // Determines if it is legal to run a sync |job|. "to run |job| by checking current operational mode, ...." http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:220: // Decide whether we should run, save or discard the job. You should use consistent terminology here with the enum. Either both should be RUN SAVE DISCARD or both should say CONTINUE SAVE DROP http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:224: // back off mode. nit - 'backoff' is one word, please check the new code for that. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:227: // Saves the job for future executio. execution. typo http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:256: void DoPendingJobIfPossible(bool is_canary_job); comment http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:260: const browser_sync::sessions::SyncSourceInfo& info); indent http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:262: void ExecuteJobByMakingACopy(SyncSessionJob* job); remove http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/session... File chrome/browser/sync/sessions/sync_session.cc (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/session... chrome/browser/sync/sessions/sync_session.cc:49: it++) { use ++it
Fixing CR feedback. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:101: DoPendingJobIfPossible(true); // set canary to true so this wont be dropped. On 2011/04/13 21:40:18, timsteele wrote: > Capitalize 'Set' > > also, see my comment @ DoCanaryJob Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:634: InitOrCoalesceJob(old_job); On 2011/04/13 21:40:18, timsteele wrote: > should include the word 'pending'Job, otherwise 'Init' is confusing :( Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:674: DoPendingJobIfPossible(true); Implemented it as discussed. Regarding your point about serverconnection event normally you wont have pending job with out wait_timer if the server connection is bad!! So most likely it is a canary job. however agree that races exist there. And the intentional decision there is it wont cause any harm. I will add a comment to that effect. On 2011/04/13 21:40:18, timsteele wrote: > Well, now I think we should just eliminate this method and > PostTask(DoPendingJobIfPossible, true) instead of posting DoCanary > > case in point - because we now call DoPendingJobIfPossible(true) from the server > connection event case, the DCHECK here isn't even valid because it now *is* > possible to have a job with is_canary_job true but no wait interval. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:46: enum JobProcessDecision { yep they were for UTs. Now I made all UTs friends. On 2011/04/13 21:40:18, timsteele wrote: > This should not be public. Only things that the public API (which is just > start/stop/schedule*) can do should be public. > > Also, comment above the enum http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:55: struct SyncSessionJob { On 2011/04/13 21:40:18, timsteele wrote: > this should not be public. Was this for tests? Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:89: struct WaitInterval { Same reason as above. Moved to private and made all uts friends. On 2011/04/13 21:40:18, timsteele wrote: > this should not be public. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:171: On 2011/04/13 21:40:18, timsteele wrote: > remove newline Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:215: // Determines if it is legal to run a sync |job|. On 2011/04/13 21:40:18, timsteele wrote: > "to run |job| by checking current operational mode, ...." Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:220: // Decide whether we should run, save or discard the job. On 2011/04/13 21:40:18, timsteele wrote: > You should use consistent terminology here with the enum. > Either both should be RUN SAVE DISCARD > or both should say CONTINUE SAVE DROP Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:224: // back off mode. On 2011/04/13 21:40:18, timsteele wrote: > nit - 'backoff' is one word, please check the new code for that. Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:227: // Saves the job for future executio. On 2011/04/13 21:40:18, timsteele wrote: > execution. typo Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:256: void DoPendingJobIfPossible(bool is_canary_job); On 2011/04/13 21:40:18, timsteele wrote: > comment Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:260: const browser_sync::sessions::SyncSourceInfo& info); On 2011/04/13 21:40:18, timsteele wrote: > indent Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:262: void ExecuteJobByMakingACopy(SyncSessionJob* job); On 2011/04/13 21:40:18, timsteele wrote: > remove Done. http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/session... File chrome/browser/sync/sessions/sync_session.cc (right): http://codereview.chromium.org/6812004/diff/16002/chrome/browser/sync/session... chrome/browser/sync/sessions/sync_session.cc:49: it++) { On 2011/04/13 21:40:18, timsteele wrote: > use ++it > Done.
almost done, mostly just style now. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:122: VLOG(1) << " Enterring start with mode = " << mode; Entering http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:147: VLOG(1) << "Doing startimpl with mode " << mode; StartImpl http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:168: VLOG(1) << "Wait interval had nudge : " << wait_interval_->had_nudge; don't think you need the VLOG(1) at the start of the second/third lines here http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:356: extra newline? http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:375: VLOG(1) << "Dropping a nudge because we are not in backoff"; this log is confusing. It's because the job was coalesced. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:379: // Re-schedule the current pending nudge. don't need both the comment and log here. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:431: VLOG(1) << "Scheduling a config"; I worry we're getting spammy with all the new logs.. maybe VLOG2? http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:510: VLOG(1) << "Dosyncsessionjob. job purpose " << job.purpose; DoSyncSessionJob http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:565: SaveJob(job); // We dont want to save polls. this comment is out of place now... did you add the if in SaveJob? http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:592: // (i.e. purpose == SyncSessionJob::CLEAR_USER_DATA), nit - comment should use up whitespace http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:722: extra newline http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:147: extra newline but also, are you sure it's not possible to change the test around to use the test class versus each having to be made a friend? http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:206: SyncSessionJob::SyncSessionJobPurpose purpose, two more spaces indent, which means the |delay| param should be brought down to line up, and the two params below should be dragged left to line up. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:230: // Determines if it is legal to run |job|. "to run |job| by checking current operational mode, backoff..." or jus get rid of 'Determines if it is legal to run job.', because it doesn't help much on its own. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:235: // Decide whether we should continue, save or discard the job. swap discard for drop http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:238: // Decide on whether to run, save or discard the job when we are in CONTINUE, SAVE, DROP http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:271: // Executes jobs that are pending. Called whenever a critical event There's only one pending job, so shouldn't be plural. And rather than "critical event", I'd say "Called whenever an event occurs that may change conditions permitting a job to run (i.e. network connectivity, mode changes, etc)."
Fixed the feeback. The function Docanaryjob has been poked a little. It does not check for the wait_interval any more. We could have a pending job with out wait interval if a nudge comes in when there is no network connectivity. In which case we dont enter exponential backoff and straight away save the job. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:122: VLOG(1) << " Enterring start with mode = " << mode; On 2011/04/13 23:31:54, timsteele wrote: > Entering Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:147: VLOG(1) << "Doing startimpl with mode " << mode; On 2011/04/13 23:31:54, timsteele wrote: > StartImpl Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:168: VLOG(1) << "Wait interval had nudge : " << wait_interval_->had_nudge; On 2011/04/13 23:31:54, timsteele wrote: > don't think you need the VLOG(1) at the start of the second/third lines here Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:356: On 2011/04/13 23:31:54, timsteele wrote: > extra newline? Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:375: VLOG(1) << "Dropping a nudge because we are not in backoff"; On 2011/04/13 23:31:54, timsteele wrote: > this log is confusing. It's because the job was coalesced. Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:379: // Re-schedule the current pending nudge. On 2011/04/13 23:31:54, timsteele wrote: > don't need both the comment and log here. Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:431: VLOG(1) << "Scheduling a config"; On 2011/04/13 23:31:54, timsteele wrote: > I worry we're getting spammy with all the new logs.. maybe VLOG2? Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:510: VLOG(1) << "Dosyncsessionjob. job purpose " << job.purpose; On 2011/04/13 23:31:54, timsteele wrote: > DoSyncSessionJob Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:565: SaveJob(job); // We dont want to save polls. On 2011/04/13 23:31:54, timsteele wrote: > this comment is out of place now... did you add the if in SaveJob? Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:592: // (i.e. purpose == SyncSessionJob::CLEAR_USER_DATA), On 2011/04/13 23:31:54, timsteele wrote: > nit - comment should use up whitespace Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:722: On 2011/04/13 23:31:54, timsteele wrote: > extra newline Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:147: I am assuming you mean to use TEST() rather than TEST_F but then I wont be able to get the fixtures. On 2011/04/13 23:31:54, timsteele wrote: > extra newline > > but also, are you sure it's not possible to change the test around to use the > test class versus each having to be made a friend? http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:206: SyncSessionJob::SyncSessionJobPurpose purpose, On 2011/04/13 23:31:54, timsteele wrote: > two more spaces indent, which means the |delay| param should be brought down to > line up, and the two params below should be dragged left to line up. Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:230: // Determines if it is legal to run |job|. On 2011/04/13 23:31:54, timsteele wrote: > "to run |job| by checking current operational mode, backoff..." > > or jus get rid of 'Determines if it is legal to run job.', because it doesn't > help much on its own. Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:235: // Decide whether we should continue, save or discard the job. On 2011/04/13 23:31:54, timsteele wrote: > swap discard for drop Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:238: // Decide on whether to run, save or discard the job when we are in On 2011/04/13 23:31:54, timsteele wrote: > CONTINUE, SAVE, DROP Done. http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:271: // Executes jobs that are pending. Called whenever a critical event On 2011/04/13 23:31:54, timsteele wrote: > There's only one pending job, so shouldn't be plural. > And rather than "critical event", I'd say "Called whenever an event occurs that > may change conditions permitting a job to run (i.e. network connectivity, mode > changes, etc)." Done.
Fixed the feeback. The function Docanaryjob has been poked a little. It does not check for the wait_interval any more. We could have a pending job with out wait interval if a nudge comes in when there is no network connectivity. In which case we dont enter exponential backoff and straight away save the job. When connection comes back up we should treat that as a canary job and execute it. The only race condition there is there could be a regular nudge scheduled at that time. But there is no harm coming treating that as canary job.(worst case one extra sync).
just one clarification below, then should be good to go http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.h (right): http://codereview.chromium.org/6812004/diff/20001/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.h:147: On 2011/04/14 00:51:07, lipalani1 wrote: > I am assuming you mean to use TEST() rather than TEST_F but then I wont be able > to get the fixtures. > On 2011/04/13 23:31:54, timsteele wrote: > > extra newline > > > > but also, are you sure it's not possible to change the test around to use the > > test class versus each having to be made a friend? > I meant adding methods to the class like SetFoo() instead of having the individual tests do syncer_thread_->foo_ = so that we don't need alllllll these friend_tests
I also updated the patch title.
http://codereview.chromium.org/6812004/diff/25001/chrome/test/live_sync/live_... File chrome/test/live_sync/live_sync_test.cc (right): http://codereview.chromium.org/6812004/diff/25001/chrome/test/live_sync/live_... chrome/test/live_sync/live_sync_test.cc:181: cl->AppendSwitch(switches::kNewSyncerThread); I'm not sure if you meant to permanently add this switch. If so, please follow the example set above (add a comment saying what it does, and add a check to see if the switch is already a part of the command line).
oh sorry it will be removed. i am running the try bot now thats why it is there. On Thu, Apr 14, 2011 at 4:19 PM, <rsimha@chromium.org> wrote: > > > http://codereview.chromium.org/6812004/diff/25001/chrome/test/live_sync/live_... > File chrome/test/live_sync/live_sync_test.cc (right): > > > http://codereview.chromium.org/6812004/diff/25001/chrome/test/live_sync/live_... > chrome/test/live_sync/live_sync_test.cc:181: > cl->AppendSwitch(switches::kNewSyncerThread); > I'm not sure if you meant to permanently add this switch. If so, please > follow the example set above (add a comment saying what it does, and add > a check to see if the switch is already a part of the command line). > > > http://codereview.chromium.org/6812004/ >
Fixing the syncbackend host.
some nits, but one legit question keeping me from approving the cl... setting server_connection_ok_ to true seems wrong... http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:78: server_connection_ok_(true), this can't be right... http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:270: VLOG(2) << this << " Saving a nudge job"; using 'this' in these is a bit messy... prefix with 'SyncerThread (" http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... File chrome/browser/sync/glue/sync_backend_host.cc (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... chrome/browser/sync/glue/sync_backend_host.cc:658: void SyncBackendHost::Core::StartConfigurationMode() { should call this 'DoStartConfigurationMode' http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... File chrome/browser/sync/glue/sync_backend_host.h (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... chrome/browser/sync/glue/sync_backend_host.h:329: void StartConfigurationMode(); move down to below the Note about Do* methods and follow style
Mac and Linux has passed with the connection set to false. waiting on windows. Rest of the feedback fixed. http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... File chrome/browser/sync/engine/syncer_thread2.cc (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:78: server_connection_ok_(true), ha ha!! Not sure if it is incorrect. However I have started a try job with that set to false. because if there is a bug there that needs to be found. We can go with it now and see what happens. On 2011/04/15 00:21:43, timsteele wrote: > this can't be right... http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/engine/... chrome/browser/sync/engine/syncer_thread2.cc:270: VLOG(2) << this << " Saving a nudge job"; On 2011/04/15 00:21:43, timsteele wrote: > using 'this' in these is a bit messy... prefix with 'SyncerThread (" Done. http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... File chrome/browser/sync/glue/sync_backend_host.cc (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... chrome/browser/sync/glue/sync_backend_host.cc:658: void SyncBackendHost::Core::StartConfigurationMode() { On 2011/04/15 00:21:43, timsteele wrote: > should call this 'DoStartConfigurationMode' Done. http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... File chrome/browser/sync/glue/sync_backend_host.h (right): http://codereview.chromium.org/6812004/diff/27004/chrome/browser/sync/glue/sy... chrome/browser/sync/glue/sync_backend_host.h:329: void StartConfigurationMode(); Oh I misunderstood the DO* convention. I thought they were thre if the parent class had a method by the same name.(there is no method in syncbackendhost by the name. startconfiguration). it is done now. On 2011/04/15 00:21:43, timsteele wrote: > move down to below the Note about Do* methods and follow style
Please review this one as well. I removed the kNewSyncerFlag which Previously I was passing so integration tests would use the new syncer flag.
The moment we've all been waiting for: LGTM
wohoo. all the bots pass as well. submitting. On Thu, Apr 14, 2011 at 7:29 PM, <tim@chromium.org> wrote: > The moment we've all been waiting for: LGTM > > > http://codereview.chromium.org/6812004/ > |
