Reduce RebaseChange max tries from 6 to 2

On failure, gerrit_util always retries HTTP requests the maximum
number of times. This doesn't always make sense, e.g. for RebaseChange
which gets 409 on a merge conflict and can't be retried into
succeeding.

Bug: b/341792235
Change-Id: I6f9e212c5b0365236a99768f056bab2eb60cddc6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5773566
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Gavin Mak <gavinmak@google.com>

gerrit_util.py

@@ -120,9 +120,9 @@ def time_time():
     return time.time()
 
 
-def log_retry_and_sleep(seconds, attempt):
+def log_retry_and_sleep(seconds, attempt, try_limit):
     LOGGER.info('Will retry in %d seconds (%d more times)...', seconds,
-                TRY_LIMIT - attempt - 1)
+                try_limit - attempt - 1)
     time_sleep(seconds)
     return seconds * random.uniform(MIN_BACKOFF, MAX_BACKOFF)
 
@@ -757,7 +757,8 @@ class GceAuthenticator(_Authenticator):
             # Retry server error status codes.
             LOGGER.warning('Encountered server error')
             if TRY_LIMIT - i > 1:
-                next_delay_sec = log_retry_and_sleep(next_delay_sec, i)
+                next_delay_sec = log_retry_and_sleep(next_delay_sec, i,
+                                                     TRY_LIMIT)
         return None, None
 
     @classmethod
@@ -968,25 +969,27 @@ def CreateHttpConn(host: str,
 
 
 def ReadHttpResponse(conn: HttpConn,
-                     accept_statuses: Container[int] = frozenset([200])):
+                     accept_statuses: Container[int] = frozenset([200]),
+                     max_tries=TRY_LIMIT):
     """Reads an HTTP response from a connection into a string buffer.
 
     Args:
         conn: An Http object created by CreateHttpConn above.
         accept_statuses: Treat any of these statuses as success. Default: [200]
             Common additions include 204, 400, and 404.
+        max_tries: The maximum number of times the request should be attempted.
     Returns:
         A string buffer containing the connection's reply.
     """
     response = contents = None
     sleep_time = SLEEP_TIME
-    for idx in range(TRY_LIMIT):
+    for idx in range(max_tries):
         before_response = time.time()
         try:
             response, contents = conn.request(**conn.req_params)
         except socket.timeout:
-            if idx < TRY_LIMIT - 1:
-                sleep_time = log_retry_and_sleep(sleep_time, idx)
+            if idx < max_tries - 1:
+                sleep_time = log_retry_and_sleep(sleep_time, idx, max_tries)
                 continue
             raise
         contents = contents.decode('utf-8', 'replace')
@@ -1024,8 +1027,8 @@ def ReadHttpResponse(conn: HttpConn,
             conn.req_params['uri'], http_version, http_version, response.status,
             response.reason, contents)
 
-        if idx < TRY_LIMIT - 1:
-            sleep_time = log_retry_and_sleep(sleep_time, idx)
+        if idx < max_tries - 1:
+            sleep_time = log_retry_and_sleep(sleep_time, idx, max_tries)
     # end of retries loop
 
     # Help the type checker a bit here - it can't figure out the `except` logic
@@ -1053,10 +1056,11 @@ def ReadHttpResponse(conn: HttpConn,
     raise GerritError(response.status, reason)
 
 
-def ReadHttpJsonResponse(
-    conn, accept_statuses: Container[int] = frozenset([200])) -> dict:
+def ReadHttpJsonResponse(conn,
+                         accept_statuses: Container[int] = frozenset([200]),
+                         max_tries=TRY_LIMIT) -> dict:
     """Parses an https response as json."""
-    fh = ReadHttpResponse(conn, accept_statuses)
+    fh = ReadHttpResponse(conn, accept_statuses, max_tries)
     # The first line of the response should always be: )]}'
     s = fh.readline()
     if s and s.rstrip() != ")]}'":
@@ -1331,7 +1335,10 @@ def RebaseChange(host, change, base=None):
     path = f'changes/{change}/rebase'
     body = {'base': base} if base else {}
     conn = CreateHttpConn(host, path, reqtype='POST', body=body)
-    return ReadHttpJsonResponse(conn)
+    # If a rebase fails due to a merge conflict, Gerrit returns 409. Retrying
+    # more than once probably won't help since the merge conflict will still
+    # exist.
+    return ReadHttpJsonResponse(conn, max_tries=2)
 
 
 def SubmitChange(host, change):

git_cl.py

@@ -4579,10 +4579,6 @@ def CMDcherry_pick(parser, args):
 
         if parent_change_num:
             try:
-                # TODO(b/341792235): gerrit_util will always retry failed Gerrit
-                # requests 5 times. This doesn't make sense if a rebase fails
-                # due to a merge conflict since the result won't change. Make
-                # RebaseChange retry at most once.
                 gerrit_util.RebaseChange(host, new_change_id, parent_change_num)
             except gerrit_util.GerritError as e:
                 parent_change_url = gerrit_util.GetChangePageUrl(

tests/gerrit_util_test.py

@@ -459,6 +459,21 @@ class GerritUtilTest(unittest.TestCase):
         self.assertEqual(2, len(conn.request.mock_calls))
         gerrit_util.time_sleep.assert_called_once_with(12.0)
 
+    def testReadHttpResponse_SetMaxTries(self):
+        conn = mock.Mock(req_params={'uri': 'uri', 'method': 'method'})
+        conn.request.side_effect = [
+            (mock.Mock(status=409), b'error!'),
+            (mock.Mock(status=409), b'error!'),
+            (mock.Mock(status=409), b'error!'),
+        ]
+
+        self.assertRaises(gerrit_util.GerritError,
+                          gerrit_util.ReadHttpResponse,
+                          conn,
+                          max_tries=2)
+        self.assertEqual(2, len(conn.request.mock_calls))
+        gerrit_util.time_sleep.assert_called_once_with(12.0)
+
     def testReadHttpResponse_Expected404(self):
         conn = mock.Mock()
         conn.req_params = {'uri': 'uri', 'method': 'method'}