Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21307002:
[android] Push only updated files in PushIfNeeded when few files have changed. (Closed)
|
Created:
7 years, 4 months ago by craigdh Modified:
7 years, 4 months ago Reviewers:
frankf CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[android] Push only updated files in PushIfNeeded when few files have changed.
BUG=263857
TEST=None
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216208
Patch Set 1 #Patch Set 2 : fix issue when pushing a single file with a different destination name #
Total comments: 18
Patch Set 3 : addressed comments #
Total comments: 11
Patch Set 4 : more comments #
Total comments: 4
Patch Set 5 : nits #Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:749: A tuple containing lists of the local and device md5sum results. Define exactly what tuple is. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:774: def GetFileDiff(self, local_path, device_path): GetFileDiff -> GetFilesChanged https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:794: def _host_has(fname): naming issue https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:800: # only a single file is given as the device_path may specify a rename. What rename? https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:801: def _h2d_path(host_path): Use our naming convention https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:807: _h2d_path(t.path) if os.path.isdir(local_path) else device_path) You're returning a list of tuples, this doesn't match the docstring https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:823: """Pushes |local_path| to |device_path|. rename local to host everywhere https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:843: def _push(local, device): Naming https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:865: if len(missing_files) > 20: This is very arbitrary. Why number of files instead of size? Why 20? Do you have empirical data?
https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:749: A tuple containing lists of the local and device md5sum results. On 2013/07/31 22:58:19, frankf wrote: > Define exactly what tuple is. Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:774: def GetFileDiff(self, local_path, device_path): On 2013/07/31 22:58:19, frankf wrote: > GetFileDiff -> GetFilesChanged Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:794: def _host_has(fname): On 2013/07/31 22:58:19, frankf wrote: > naming issue Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:800: # only a single file is given as the device_path may specify a rename. On 2013/07/31 22:58:19, frankf wrote: > What rename? If the local_path and device_path refer to a file but have a different base name then the file is being renamed. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:801: def _h2d_path(host_path): On 2013/07/31 22:58:19, frankf wrote: > Use our naming convention Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:807: _h2d_path(t.path) if os.path.isdir(local_path) else device_path) On 2013/07/31 22:58:19, frankf wrote: > You're returning a list of tuples, this doesn't match the docstring Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:823: """Pushes |local_path| to |device_path|. On 2013/07/31 22:58:19, frankf wrote: > rename local to host everywhere Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:843: def _push(local, device): On 2013/07/31 22:58:19, frankf wrote: > Naming Done. https://codereview.chromium.org/21307002/diff/7001/build/android/pylib/androi... build/android/pylib/android_commands.py:865: if len(missing_files) > 20: On 2013/07/31 22:58:19, frankf wrote: > This is very arbitrary. Why number of files instead of size? Why 20? Do you have > empirical data? Based on size for now with a cutoff and added a TODO. I plan on working out the heuristic for a follow up cl.
https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:187: def _ComputeFileListHash(md5sum_output): Rename this to _ParseMd5sumOutput() https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:194: List of namedtuples (hash, path). What is path? Is it absolute or relative? Provide an example https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:741: def _RunMd5Sum(self, host_path, device_path): Why not inline this in GetFilesChanged? https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:813: def CheckMd5Sum(self, host_path, device_path): Can you get rid of this and just call GetFilesChanged? https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:843: missing_files = self.GetFilesChanged(host_path, device_path) this is misleading -> changed_files https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:873: # approximates the push time for each method. Can you look how adb sync operates: https://android.googlesource.com/platform/system/core/+/master/adb/file_sync_...
https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:194: List of namedtuples (hash, path). On 2013/08/06 00:17:23, frankf wrote: > What is path? Is it absolute or relative? Provide an example Done. https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:741: def _RunMd5Sum(self, host_path, device_path): On 2013/08/06 00:17:23, frankf wrote: > Why not inline this in GetFilesChanged? Could, this seems reusable and keeps the functions each to readable lengths though. https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:813: def CheckMd5Sum(self, host_path, device_path): On 2013/08/06 00:17:23, frankf wrote: > Can you get rid of this and just call GetFilesChanged? Done. https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:843: missing_files = self.GetFilesChanged(host_path, device_path) On 2013/08/06 00:17:23, frankf wrote: > this is misleading -> changed_files Done. https://codereview.chromium.org/21307002/diff/16001/build/android/pylib/andro... build/android/pylib/android_commands.py:873: # approximates the push time for each method. On 2013/08/06 00:17:23, frankf wrote: > Can you look how adb sync operates: > https://android.googlesource.com/platform/system/core/+/master/adb/file_sync_... Seems to just compare the timestamps. From what I gather it doesn't use adb push as it is directly reading in the files and sending data to adbd on the device to write out.
lgtm w/ nit https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... build/android/pylib/android_commands.py:195: absolute path to the file with an Md5Sum of hash. Add || around parameters https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... build/android/pylib/android_commands.py:849: if 'resource busy' in output and retry < 3: maybe just remove 'resource busy' in output and
https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... File build/android/pylib/android_commands.py (right): https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... build/android/pylib/android_commands.py:195: absolute path to the file with an Md5Sum of hash. On 2013/08/06 21:43:57, frankf wrote: > Add || around parameters Done. https://codereview.chromium.org/21307002/diff/25001/build/android/pylib/andro... build/android/pylib/android_commands.py:849: if 'resource busy' in output and retry < 3: On 2013/08/06 21:43:57, frankf wrote: > maybe just remove 'resource busy' in output and Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/21307002/30001
Message was sent while issue was closed.
Change committed as 216208 |
