[SyncFileSystem] Push metadata with empty MD5 for to_be_fetched file.

[SyncFileSystem] Push metadata with empty MD5 for to_be_fetched file.

Populating this for not-yet-fetched file causes NOT_MODIFIED case on downloading the file.

BUG=229764
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203097

[tzik, 2013-05-29 06:02:11]

PTL

[kinuko, 2013-05-29 06:10:47]

Can you fill the BUG= line?  Is there a way for us to find out these issues in
tests? Should we raise the priority for end-to-end tests?

The change looks good

[tzik, 2013-05-29 06:27:43]

On 2013/05/29 06:10:47, kinuko wrote:
> Can you fill the BUG= line?  Is there a way for us to find out these issues in
> tests? Should we raise the priority for end-to-end tests?
> 
> The change looks good

I think this should be cached by DriveFileSyncServiceSyncTest, but was not.
Maybe, FakeAPIUtil was too simple for this case.

[kinuko, 2013-05-29 06:34:27]

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
metadata.set_to_be_fetched(true);
Question... if to_be_fetched==true should md5_checksum always empty? Then maybe
we can have additional check somewhere to enforce it?

[calvinlo, 2013-05-29 06:39:00]

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:773:
metadata.set_md5_checksum(std::string());
While this change works, I think it's honestly quite hacky. It forces the coder
to know that inside api_util L696 that there's an MD5 check that returns early
when it thinks the contents on drive and local match.

Instead of trying to workaround this if condition by setting the md5_checksum to
be empty here, I would suggest adding an extra param to DownloadFile. something
like bool (to_be_fetched), or (initial_download), (force_download), etc. Or as
an alternative, have two versions of the function, one that does the MD5 check
and another which doesn't.

Then go back and change DriveFileSyncService L1015 to use the value of
to_be_fetched.

I think if you do this, you'll also be able to remove lines 1008-1013.

[tzik, 2013-05-29 06:41:53]

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
metadata.set_to_be_fetched(true);
On 2013/05/29 06:34:28, kinuko wrote:
> Question... if to_be_fetched==true should md5_checksum always empty? Then
maybe
> we can have additional check somewhere to enforce it?

Yes, we should check it.
I think the best places are in DriveMetadataStore::{ReadEntry,UpdateEntry}.

[calvinlo, 2013-05-29 06:42:33]

On 2013/05/29 06:34:27, kinuko wrote:
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):
> 
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
> metadata.set_to_be_fetched(true);
> Question... if to_be_fetched==true should md5_checksum always empty? Then
maybe
> we can have additional check somewhere to enforce it?

This seems to be the case. Unless I'm mistaken, the only way to really enforce
this would be to wrap both setters so that it always checks the value of the
other variable. This doesn't seem ideal because DriveMetadata is a protobuf with
autogenerated setters and getters.

I also think while this could work, it's not as clear as the solution I
alternatively suggested.

[tzik, 2013-05-29 07:08:11]

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:773:
metadata.set_md5_checksum(std::string());
On 2013/05/29 06:39:00, calvinlo wrote:
> While this change works, I think it's honestly quite hacky. It forces the
coder
> to know that inside api_util L696 that there's an MD5 check that returns early
> when it thinks the contents on drive and local match.
> 
> Instead of trying to workaround this if condition by setting the md5_checksum
to
> be empty here, I would suggest adding an extra param to DownloadFile.
something
> like bool (to_be_fetched), or (initial_download), (force_download), etc. Or as
> an alternative, have two versions of the function, one that does the MD5 check
> and another which doesn't.
> 
> Then go back and change DriveFileSyncService L1015 to use the value of
> to_be_fetched.
> 
> I think if you do this, you'll also be able to remove lines 1008-1013. 

This does not need .cc impl, but needs the documentation on
api_util_interface.h.
And, I think we all should know the function we call.

Adding another flag to DownloadFile in this case will needs another
to_be_fetched flag handling in the download function.
That looks not cleaner to me.

The md5_checksum should hold the md5 value of the file we fetched last time.
Which we should detects the remote change.
So, I believe, setting this value to empty string is not so hacky.

[calvinlo, 2013-05-29 07:50:09]

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:773:
metadata.set_md5_checksum(std::string());
On 2013/05/29 07:08:11, tzik wrote:
> On 2013/05/29 06:39:00, calvinlo wrote:
> > While this change works, I think it's honestly quite hacky. It forces the
> coder
> > to know that inside api_util L696 that there's an MD5 check that returns
early
> > when it thinks the contents on drive and local match.
> > 
> > Instead of trying to workaround this if condition by setting the
md5_checksum
> to
> > be empty here, I would suggest adding an extra param to DownloadFile.
> something
> > like bool (to_be_fetched), or (initial_download), (force_download), etc. Or
as
> > an alternative, have two versions of the function, one that does the MD5
check
> > and another which doesn't.
> > 
> > Then go back and change DriveFileSyncService L1015 to use the value of
> > to_be_fetched.
> > 
> > I think if you do this, you'll also be able to remove lines 1008-1013. 
> 
> This does not need .cc impl, but needs the documentation on
> api_util_interface.h.
> And, I think we all should know the function we call.
> 
> Adding another flag to DownloadFile in this case will needs another
> to_be_fetched flag handling in the download function.
> That looks not cleaner to me.
> 
> The md5_checksum should hold the md5 value of the file we fetched last time.
> Which we should detects the remote change.
> So, I believe, setting this value to empty string is not so hacky.

I'm not sure that I agree. Right now the function is called DownloadFile but it
doesn't give you an indication that it will actually not download the file if
the given md5 matches the existing one.

A suggestion, you can make one function called
DownloadFile() which doesn't take in the MD5 and always downloads the file. Then
add another DownloadFileIfUpdated which does take in the MD5. The function names
and parameters make the code self documenting.

https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
metadata.set_to_be_fetched(true);
On 2013/05/29 06:41:53, tzik wrote:
> On 2013/05/29 06:34:28, kinuko wrote:
> > Question... if to_be_fetched==true should md5_checksum always empty? Then
> maybe
> > we can have additional check somewhere to enforce it?
> 
> Yes, we should check it.
> I think the best places are in DriveMetadataStore::{ReadEntry,UpdateEntry}. 

If you go this route, I think it'd also be a good idea to note this in the
protobuf. i.e. md5_checksum is only populated once the file has been downloaded
and if it's empty then to_be_fetched must be true.

[kinuko, 2013-05-29 14:50:14]

On 2013/05/29 07:50:09, calvinlo wrote:
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):
> 
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> chrome/browser/sync_file_system/drive_file_sync_service.cc:773:
> metadata.set_md5_checksum(std::string());
> On 2013/05/29 07:08:11, tzik wrote:
> > On 2013/05/29 06:39:00, calvinlo wrote:
> > > While this change works, I think it's honestly quite hacky. It forces the
> > coder
> > > to know that inside api_util L696 that there's an MD5 check that returns
> early
> > > when it thinks the contents on drive and local match.
> > > 
> > > Instead of trying to workaround this if condition by setting the
> md5_checksum
> > to
> > > be empty here, I would suggest adding an extra param to DownloadFile.
> > something
> > > like bool (to_be_fetched), or (initial_download), (force_download), etc.
Or
> as
> > > an alternative, have two versions of the function, one that does the MD5
> check
> > > and another which doesn't.
> > > 
> > > Then go back and change DriveFileSyncService L1015 to use the value of
> > > to_be_fetched.
> > > 
> > > I think if you do this, you'll also be able to remove lines 1008-1013. 
> > 
> > This does not need .cc impl, but needs the documentation on
> > api_util_interface.h.
> > And, I think we all should know the function we call.
> > 
> > Adding another flag to DownloadFile in this case will needs another
> > to_be_fetched flag handling in the download function.
> > That looks not cleaner to me.
> > 
> > The md5_checksum should hold the md5 value of the file we fetched last time.
> > Which we should detects the remote change.
> > So, I believe, setting this value to empty string is not so hacky.
> 
> I'm not sure that I agree. Right now the function is called DownloadFile but
it
> doesn't give you an indication that it will actually not download the file if
> the given md5 matches the existing one.
> 
> A suggestion, you can make one function called
> DownloadFile() which doesn't take in the MD5 and always downloads the file.
Then
> add another DownloadFileIfUpdated which does take in the MD5. The function
names
> and parameters make the code self documenting.
> 
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
> metadata.set_to_be_fetched(true);
> On 2013/05/29 06:41:53, tzik wrote:
> > On 2013/05/29 06:34:28, kinuko wrote:
> > > Question... if to_be_fetched==true should md5_checksum always empty? Then
> > maybe
> > > we can have additional check somewhere to enforce it?
> > 
> > Yes, we should check it.
> > I think the best places are in DriveMetadataStore::{ReadEntry,UpdateEntry}. 

Do you want to submit this and leave the rest of the changes (adding consistency
check in UpdateEntry and tests) in a separate patch?
I can stamp this one to let it go or can wait.
In either ways can you reopen the issue 229764 and update what needs to be
fixed/changed further?

> If you go this route, I think it'd also be a good idea to note this in the
> protobuf. i.e. md5_checksum is only populated once the file has been
downloaded
> and if it's empty then to_be_fetched must be true.

The reverse isn't true, there's a case where md5 is empty and to_be_fetched is
false.

[kinuko, 2013-05-30 02:13:55]

(Talked offline /w tzik)
Ok, let this go as is as a quick fix, since the regression looks serious. LGTM

Let's reopen the original issue for fixing
DriveMetadataStore::{ReadEntry,UpdateEntry} and adding tests.

(I do not want to change the DownloadFile interface at this stage, let's revisit
after we finish higher priority fixes)

[tzik, 2013-05-30 03:49:03]

On 2013/05/29 07:50:09, calvinlo wrote:
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):
> 
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> chrome/browser/sync_file_system/drive_file_sync_service.cc:773:
> metadata.set_md5_checksum(std::string());
> On 2013/05/29 07:08:11, tzik wrote:
> > On 2013/05/29 06:39:00, calvinlo wrote:
> > > While this change works, I think it's honestly quite hacky. It forces the
> > coder
> > > to know that inside api_util L696 that there's an MD5 check that returns
> early
> > > when it thinks the contents on drive and local match.
> > > 
> > > Instead of trying to workaround this if condition by setting the
> md5_checksum
> > to
> > > be empty here, I would suggest adding an extra param to DownloadFile.
> > something
> > > like bool (to_be_fetched), or (initial_download), (force_download), etc.
Or
> as
> > > an alternative, have two versions of the function, one that does the MD5
> check
> > > and another which doesn't.
> > > 
> > > Then go back and change DriveFileSyncService L1015 to use the value of
> > > to_be_fetched.
> > > 
> > > I think if you do this, you'll also be able to remove lines 1008-1013. 
> > 
> > This does not need .cc impl, but needs the documentation on
> > api_util_interface.h.
> > And, I think we all should know the function we call.
> > 
> > Adding another flag to DownloadFile in this case will needs another
> > to_be_fetched flag handling in the download function.
> > That looks not cleaner to me.
> > 
> > The md5_checksum should hold the md5 value of the file we fetched last time.
> > Which we should detects the remote change.
> > So, I believe, setting this value to empty string is not so hacky.
> 
> I'm not sure that I agree. Right now the function is called DownloadFile but
it
> doesn't give you an indication that it will actually not download the file if
> the given md5 matches the existing one.

It does as the document.

> 
> A suggestion, you can make one function called
> DownloadFile() which doesn't take in the MD5 and always downloads the file.
Then
> add another DownloadFileIfUpdated which does take in the MD5. The function
names
> and parameters make the code self documenting.

Might be better but that can be in separate thing than the bugfix.

> 
>
https://codereview.chromium.org/16152003/diff/1/chrome/browser/sync_file_syst...
> chrome/browser/sync_file_system/drive_file_sync_service.cc:775:
> metadata.set_to_be_fetched(true);
> On 2013/05/29 06:41:53, tzik wrote:
> > On 2013/05/29 06:34:28, kinuko wrote:
> > > Question... if to_be_fetched==true should md5_checksum always empty? Then
> > maybe
> > > we can have additional check somewhere to enforce it?
> > 
> > Yes, we should check it.
> > I think the best places are in DriveMetadataStore::{ReadEntry,UpdateEntry}. 
> 
> If you go this route, I think it'd also be a good idea to note this in the
> protobuf. i.e. md5_checksum is only populated once the file has been
downloaded
> and if it's empty then to_be_fetched must be true.

[commit-bot: I haz the power, 2013-05-30 03:49:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/16152003/1

[calvinlo, 2013-05-30 03:57:06]

lgtm

ok for now then based on comments.

[commit-bot: I haz the power, 2013-05-30 06:36:11]

Change committed as 203097