Gerrit git cl: cache change detail to save on RPC calls.

R=sergiyb@chromium.org
BUG=681704

Change-Id: I5c320f7e6442a92ae0574e2ca6481a082e82568a
Reviewed-on: https://chromium-review.googlesource.com/430795
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>

git_cl.py

@@ -2270,6 +2270,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     # Lazily cached values.
     self._gerrit_server = None  # e.g. https://chromium-review.googlesource.com
     self._gerrit_host = None    # e.g. chromium-review.googlesource.com
+    # Map from change number (issue) to its detail cache.
+    self._detail_cache = {}
 
   def _GetGerritHost(self):
     # Lazy load of configs.
@@ -2487,10 +2489,36 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     gerrit_util.SubmitChange(self._GetGerritHost(), self.GetIssue(),
                              wait_for_merge=wait_for_merge)
 
-  def _GetChangeDetail(self, options=None, issue=None):
+  def _GetChangeDetail(self, options=None, issue=None,
+                       no_cache=False):
+    """Returns details of the issue by querying Gerrit and caching results.
+
+    If fresh data is needed, set no_cache=True which will clear cache and
+    thus new data will be fetched from Gerrit.
+    """
     options = options or []
     issue = issue or self.GetIssue()
     assert issue, 'issue is required to query Gerrit'
+
+    # Normalize issue and options for consistent keys in cache.
+    issue = str(issue)
+    options = [o.upper() for o in options]
+
+    # Check in cache first unless no_cache is True.
+    if no_cache:
+      self._detail_cache.pop(issue, None)
+    else:
+      options_set = frozenset(options)
+      for cached_options_set, data in self._detail_cache.get(issue, []):
+        # Assumption: data fetched before with extra options is suitable
+        # for return for a smaller set of options.
+        # For example, if we cached data for
+        #     options=[CURRENT_REVISION, DETAILED_FOOTERS]
+        #   and request is for options=[CURRENT_REVISION],
+        # THEN we can return prior cached data.
+        if options_set.issubset(cached_options_set):
+          return data
+
     try:
       data = gerrit_util.GetChangeDetail(self._GetGerritHost(), str(issue),
                                          options, ignore_404=False)
@@ -2498,6 +2526,8 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
       if e.http_status == 404:
         raise GerritChangeNotExists(issue, self.GetCodereviewServer())
       raise
+
+    self._detail_cache.setdefault(issue, []).append((frozenset(options), data))
     return data
 
   def _GetChangeCommit(self, issue=None):

tests/git_cl_test.py

@@ -2794,6 +2794,63 @@ class TestGitCl(TestCase):
     self.assertRegexpMatches(sys.stdout.getvalue(), 'Started:')
     self.assertRegexpMatches(sys.stdout.getvalue(), '2 try jobs')
 
+  def _mock_gerrit_changes_for_detail_cache(self):
+    self.mock(git_cl._GerritChangelistImpl, '_GetGerritHost', lambda _: 'host')
+    self.mock(git_cl.gerrit_util, 'GetChangeDetail',
+              lambda _, issue, options, **__:
+                  self._mocked_call('RPC', issue, options))
+
+  def test_gerrit_change_detail_cache_normalize(self):
+    self._mock_gerrit_changes_for_detail_cache()
+    self.calls = [
+        (('RPC', '2', ['CASE']), 'b'),
+    ]
+    cl = git_cl.Changelist(codereview='gerrit')
+    self.assertEqual(cl._GetChangeDetail(issue=2, options=['CaSe']), 'b')
+    self.assertEqual(cl._GetChangeDetail(issue=2, options=['CASE']), 'b')
+    self.assertEqual(cl._GetChangeDetail(issue='2'), 'b')
+    self.assertEqual(cl._GetChangeDetail(issue=2), 'b')
+
+  def test_gerrit_change_detail_cache_simple(self):
+    self._mock_gerrit_changes_for_detail_cache()
+    self.calls = [
+        (('RPC', '1', []), 'a'),
+        (('RPC', '2', []), 'b'),
+        (('RPC', '2', []), 'b2'),
+    ]
+    cl = git_cl.Changelist(issue=1, codereview='gerrit')
+    self.assertEqual(cl._GetChangeDetail(), 'a')  # Miss.
+    self.assertEqual(cl._GetChangeDetail(), 'a')
+    self.assertEqual(cl._GetChangeDetail(issue=2), 'b')  # Miss.
+    self.assertEqual(cl._GetChangeDetail(issue=2, no_cache=True), 'b2') # Miss.
+    self.assertEqual(cl._GetChangeDetail(), 'a')
+    self.assertEqual(cl._GetChangeDetail(issue=2), 'b2')
+
+  def test_gerrit_change_detail_cache_options(self):
+    self._mock_gerrit_changes_for_detail_cache()
+    self.calls = [
+        (('RPC', '1', ['C', 'A', 'B']), 'cab'),
+        (('RPC', '1', ['A', 'D']), 'ad'),
+        (('RPC', '1', ['A']), 'a'),  # no_cache=True
+        (('RPC', '1', ['B']), 'b'),  # no longer in cache.
+    ]
+    cl = git_cl.Changelist(issue=1, codereview='gerrit')
+    self.assertEqual(cl._GetChangeDetail(options=['C', 'A', 'B']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(options=['A', 'B', 'C']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(options=['B', 'A']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(options=['C']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(options=['A']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(), 'cab')
+
+    self.assertEqual(cl._GetChangeDetail(options=['A', 'D']), 'ad')
+    self.assertEqual(cl._GetChangeDetail(options=['A']), 'cab')
+    self.assertEqual(cl._GetChangeDetail(options=['D']), 'ad')
+    self.assertEqual(cl._GetChangeDetail(), 'cab')
+
+    # Finally, no_cache should invalidate all caches for given change.
+    self.assertEqual(cl._GetChangeDetail(options=['A'], no_cache=True), 'a')
+    self.assertEqual(cl._GetChangeDetail(options=['B']), 'b')
+
 
 if __name__ == '__main__':
   git_cl.logging.basicConfig(