{"action":"created","comment":{"url":"https://api.github.com/repos/youtube/vitess/pulls/comments/40052360","id":40052360,"diff_hunk":"@@ -298,60 +298,91 @@ def _execute_entity_ids(\n       raise\n     return results, rowcount, lastrowid, fields\n \n+  # keyspace_ids and keyranges aod make two different batch calls if\n+  # there are some params that use both.\n   @vtgate_utils.exponential_backoff_retry((dbexceptions.RequestBacklog))\n   def _execute_batch(\n       self, sql_list, bind_variables_list, keyspace_list, keyspace_ids_list,\n-      tablet_type, as_transaction, effective_caller_id=None):\n-    query_list = []\n-    for sql, bind_vars, keyspace, keyspace_ids in zip(\n-        sql_list, bind_variables_list, keyspace_list, keyspace_ids_list):\n+      shards_list, tablet_type, as_transaction, effective_caller_id=None):\n+    keyspace_ids_query_list = []\n+    shards_query_list = []\n+    is_keyspace_ids_list = []\n+    for sql, bind_vars, keyspace, keyspace_ids, shards in zip(\n+        sql_list, bind_variables_list, keyspace_list, keyspace_ids_list,\n+        shards_list):\n       sql, bind_vars = dbapi.prepare_query_bind_vars(sql, bind_vars)\n       query = {}\n       query['Sql'] = sql\n       query['BindVariables'] = field_types.convert_bind_vars(bind_vars)\n       query['Keyspace'] = keyspace\n-      query['KeyspaceIds'] = keyspace_ids\n-      query_list.append(query)\n+      is_keyspace_ids = bool(keyspace_ids)\n+      is_keyspace_ids_list.append(is_keyspace_ids)\n+      if is_keyspace_ids:\n+        if shards:\n+          raise dbexceptions.ProgrammingError(\n+              'Keyspace_ids and shards both defined in same params.')\n+        query['KeyspaceIds'] = keyspace_ids\n+        keyspace_ids_query_list.append(query)\n+      else:\n+        query['Shards'] = shards\n+        shards_query_list.append(query)\n+\n+    def make_execute_batch_call(query_list, is_keyspace_ids):\n+      rowsets = []\n+      if not query_list:\n+        return rowsets\n+      try:\n+        req = {\n+            'Queries': query_list,\n+            'TabletType': tablet_type,\n+            'AsTransaction': as_transaction,\n+        }\n+        self._add_caller_id(req, effective_caller_id)\n+        self._add_session(req)\n+        if is_keyspace_ids:\n+          exec_method = 'VTGate.ExecuteBatchKeyspaceIds'\n+        else:\n+          exec_method = 'VTGate.ExecuteBatchShard'\n+        response = self._get_client().call(exec_method, req)\n+        self._update_session(response)\n+        if response.reply.get('Error'):\n+          raise gorpc.AppError(response.reply['Error'], exec_method)\n+        for reply in response.reply['List']:\n+          fields = []\n+          conversions = []\n+          results = []\n+          rowcount = 0\n+          for field in reply['Fields']:\n+            fields.append((field['Name'], field['Type']))\n+            conversions.append(field_types.conversions.get(field['Type']))\n+          for row in reply['Rows']:\n+            results.append(tuple(_make_row(row, conversions)))\n+          rowcount = reply['RowsAffected']\n+          lastrowid = reply['InsertId']\n+          rowsets.append((results, rowcount, lastrowid, fields))\n+      except gorpc.GoRpcError as e:\n+        self.logger_object.log_private_data(bind_variables_list)\n+        raise self._convert_exception(\n+            e, sql_list, exec_method,\n+            keyspace='', tablet_type=tablet_type)\n+      except Exception:\n+        logging.exception('gorpc low-level error')\n+        raise\n+      return rowsets\n \n+    # Return merged keyspace_ids and shards rowsets.\n     rowsets = []\n-\n-    try:\n-      req = {\n-          'Queries': query_list,\n-          'TabletType': tablet_type,\n-          'AsTransaction': as_transaction,\n-      }\n-      self._add_caller_id(req, effective_caller_id)\n-      self._add_session(req)\n-      response = self._get_client().call('VTGate.ExecuteBatchKeyspaceIds', req)\n-      self._update_session(response)\n-      if response.reply.get('Error'):\n-        raise gorpc.AppError(\n-            response.reply['Error'], 'VTGate.ExecuteBatchKeyspaceIds')\n-      for reply in response.reply['List']:\n-        fields = []\n-        conversions = []\n-        results = []\n-        rowcount = 0\n-\n-        for field in reply['Fields']:\n-          fields.append((field['Name'], field['Type']))\n-          conversions.append(field_types.conversions.get(field['Type']))\n-\n-        for row in reply['Rows']:\n-          results.append(tuple(_make_row(row, conversions)))\n-\n-        rowcount = reply['RowsAffected']\n-        lastrowid = reply['InsertId']\n-        rowsets.append((results, rowcount, lastrowid, fields))\n-    except gorpc.GoRpcError as e:\n-      self.logger_object.log_private_data(bind_variables_list)\n-      raise self._convert_exception(\n-          e, sql_list, keyspace_ids_list,\n-          keyspace='', tablet_type=tablet_type)\n-    except Exception:\n-      logging.exception('gorpc low-level error')\n-      raise\n+    keyspace_ids_rowsets = make_execute_batch_call(\n+        keyspace_ids_query_list, is_keyspace_ids=True)\n+    shards_rowsets = make_execute_batch_call(\n+        shards_query_list, is_keyspace_ids=False)\n+    keyspace_ids_iter = iter(keyspace_ids_rowsets)\n+    shards_iter = iter(shards_rowsets)\n+    for is_keyspace_ids in is_keyspace_ids_list:\n+      if is_keyspace_ids:\n+        rowsets.append(keyspace_ids_iter.next())\n+      else:\n+        rowsets.append(shards_iter.next())","path":"py/vtdb/vtgatev2.py","position":132,"original_position":132,"commit_id":"3ba4d9d2e9609793c80b735256e74bca488c95ae","original_commit_id":"3ba4d9d2e9609793c80b735256e74bca488c95ae","user":{"login":"sougou","id":589370,"avatar_url":"https://avatars.githubusercontent.com/u/589370?v=3","gravatar_id":"","url":"https://api.github.com/users/sougou","html_url":"https://github.com/sougou","followers_url":"https://api.github.com/users/sougou/followers","following_url":"https://api.github.com/users/sougou/following{/other_user}","gists_url":"https://api.github.com/users/sougou/gists{/gist_id}","starred_url":"https://api.github.com/users/sougou/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sougou/subscriptions","organizations_url":"https://api.github.com/users/sougou/orgs","repos_url":"https://api.github.com/users/sougou/repos","events_url":"https://api.github.com/users/sougou/events{/privacy}","received_events_url":"https://api.github.com/users/sougou/received_events","type":"User","site_admin":false},"body":"Would it be better to have two separate functions/code paths for the two? I'm thinking the only thing common between the two is the result processing, which can be reused as a function.\r\nOtherwise, these sprinkled conditionals don't feel readable.","created_at":"2015-09-22T04:30:49Z","updated_at":"2015-09-22T04:30:49Z","html_url":"https://github.com/youtube/vitess/pull/1120#discussion_r40052360","pull_request_url":"https://api.github.com/repos/youtube/vitess/pulls/1120","_links":{"self":{"href":"https://api.github.com/repos/youtube/vitess/pulls/comments/40052360"},"html":{"href":"https://github.com/youtube/vitess/pull/1120#discussion_r40052360"},"pull_request":{"href":"https://api.github.com/repos/youtube/vitess/pulls/1120"}}},"pull_request":{"url":"https://api.github.com/repos/youtube/vitess/pulls/1120","id":45462483,"html_url":"https://github.com/youtube/vitess/pull/1120","diff_url":"https://github.com/youtube/vitess/pull/1120.diff","patch_url":"https://github.com/youtube/vitess/pull/1120.patch","issue_url":"https://api.github.com/repos/youtube/vitess/issues/1120","number":1120,"state":"open","locked":false,"title":"Support ExecuteBatchShard in vtgatev2.py.","user":{"login":"dumbunny","id":11982482,"avatar_url":"https://avatars.githubusercontent.com/u/11982482?v=3","gravatar_id":"","url":"https://api.github.com/users/dumbunny","html_url":"https://github.com/dumbunny","followers_url":"https://api.github.com/users/dumbunny/followers","following_url":"https://api.github.com/users/dumbunny/following{/other_user}","gists_url":"https://api.github.com/users/dumbunny/gists{/gist_id}","starred_url":"https://api.github.com/users/dumbunny/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dumbunny/subscriptions","organizations_url":"https://api.github.com/users/dumbunny/orgs","repos_url":"https://api.github.com/users/dumbunny/repos","events_url":"https://api.github.com/users/dumbunny/events{/privacy}","received_events_url":"https://api.github.com/users/dumbunny/received_events","type":"User","site_admin":false},"body":"Change the vtgateclienttest/services/callerid.go test to always return\r\nan error when callerid is handled. The error will contain word\r\n\"SUCCESS:\" if callerid matches. This is necessary since falling through\r\nbreaks the expectations of the caller.\r\n\r\nIn the connection's _execute_batch call, some routing will be\r\nkeyspace_ids, and some will be by shards. This means that an\r\n_execute_batch may create both a ExecuteBatchKeyspaceIds and\r\nExecuteBatchShard call. Merge the results back together.\r\n\r\nAdd tests for ExecuteBatchShard in python_client_test.py and\r\nvtgatev2_test.py.","created_at":"2015-09-18T23:19:11Z","updated_at":"2015-09-22T04:30:49Z","closed_at":null,"merged_at":null,"merge_commit_sha":"f8d0b013f847947ced7bb006bdc75352ef5678d8","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/youtube/vitess/pulls/1120/commits","review_comments_url":"https://api.github.com/repos/youtube/vitess/pulls/1120/comments","review_comment_url":"https://api.github.com/repos/youtube/vitess/pulls/comments{/number}","comments_url":"https://api.github.com/repos/youtube/vitess/issues/1120/comments","statuses_url":"https://api.github.com/repos/youtube/vitess/statuses/3ba4d9d2e9609793c80b735256e74bca488c95ae","head":{"label":"dumbunny:batch","ref":"batch","sha":"3ba4d9d2e9609793c80b735256e74bca488c95ae","user":{"login":"dumbunny","id":11982482,"avatar_url":"https://avatars.githubusercontent.com/u/11982482?v=3","gravatar_id":"","url":"https://api.github.com/users/dumbunny","html_url":"https://github.com/dumbunny","followers_url":"https://api.github.com/users/dumbunny/followers","following_url":"https://api.github.com/users/dumbunny/following{/other_user}","gists_url":"https://api.github.com/users/dumbunny/gists{/gist_id}","starred_url":"https://api.github.com/users/dumbunny/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dumbunny/subscriptions","organizations_url":"https://api.github.com/users/dumbunny/orgs","repos_url":"https://api.github.com/users/dumbunny/repos","events_url":"https://api.github.com/users/dumbunny/events{/privacy}","received_events_url":"https://api.github.com/users/dumbunny/received_events","type":"User","site_admin":false},"repo":{"id":39525824,"name":"vitess","full_name":"dumbunny/vitess","owner":{"login":"dumbunny","id":11982482,"avatar_url":"https://avatars.githubusercontent.com/u/11982482?v=3","gravatar_id":"","url":"https://api.github.com/users/dumbunny","html_url":"https://github.com/dumbunny","followers_url":"https://api.github.com/users/dumbunny/followers","following_url":"https://api.github.com/users/dumbunny/following{/other_user}","gists_url":"https://api.github.com/users/dumbunny/gists{/gist_id}","starred_url":"https://api.github.com/users/dumbunny/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dumbunny/subscriptions","organizations_url":"https://api.github.com/users/dumbunny/orgs","repos_url":"https://api.github.com/users/dumbunny/repos","events_url":"https://api.github.com/users/dumbunny/events{/privacy}","received_events_url":"https://api.github.com/users/dumbunny/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/dumbunny/vitess","description":"vitess provides servers and tools which facilitate scaling of MySQL databases for large scale web services.","fork":true,"url":"https://api.github.com/repos/dumbunny/vitess","forks_url":"https://api.github.com/repos/dumbunny/vitess/forks","keys_url":"https://api.github.com/repos/dumbunny/vitess/keys{/key_id}","collaborators_url":"https://api.github.com/repos/dumbunny/vitess/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/dumbunny/vitess/teams","hooks_url":"https://api.github.com/repos/dumbunny/vitess/hooks","issue_events_url":"https://api.github.com/repos/dumbunny/vitess/issues/events{/number}","events_url":"https://api.github.com/repos/dumbunny/vitess/events","assignees_url":"https://api.github.com/repos/dumbunny/vitess/assignees{/user}","branches_url":"https://api.github.com/repos/dumbunny/vitess/branches{/branch}","tags_url":"https://api.github.com/repos/dumbunny/vitess/tags","blobs_url":"https://api.github.com/repos/dumbunny/vitess/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/dumbunny/vitess/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/dumbunny/vitess/git/refs{/sha}","trees_url":"https://api.github.com/repos/dumbunny/vitess/git/trees{/sha}","statuses_url":"https://api.github.com/repos/dumbunny/vitess/statuses/{sha}","languages_url":"https://api.github.com/repos/dumbunny/vitess/languages","stargazers_url":"https://api.github.com/repos/dumbunny/vitess/stargazers","contributors_url":"https://api.github.com/repos/dumbunny/vitess/contributors","subscribers_url":"https://api.github.com/repos/dumbunny/vitess/subscribers","subscription_url":"https://api.github.com/repos/dumbunny/vitess/subscription","commits_url":"https://api.github.com/repos/dumbunny/vitess/commits{/sha}","git_commits_url":"https://api.github.com/repos/dumbunny/vitess/git/commits{/sha}","comments_url":"https://api.github.com/repos/dumbunny/vitess/comments{/number}","issue_comment_url":"https://api.github.com/repos/dumbunny/vitess/issues/comments{/number}","contents_url":"https://api.github.com/repos/dumbunny/vitess/contents/{+path}","compare_url":"https://api.github.com/repos/dumbunny/vitess/compare/{base}...{head}","merges_url":"https://api.github.com/repos/dumbunny/vitess/merges","archive_url":"https://api.github.com/repos/dumbunny/vitess/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/dumbunny/vitess/downloads","issues_url":"https://api.github.com/repos/dumbunny/vitess/issues{/number}","pulls_url":"https://api.github.com/repos/dumbunny/vitess/pulls{/number}","milestones_url":"https://api.github.com/repos/dumbunny/vitess/milestones{/number}","notifications_url":"https://api.github.com/repos/dumbunny/vitess/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/dumbunny/vitess/labels{/name}","releases_url":"https://api.github.com/repos/dumbunny/vitess/releases{/id}","created_at":"2015-07-22T19:24:02Z","updated_at":"2015-07-22T19:24:05Z","pushed_at":"2015-09-21T23:17:00Z","git_url":"git://github.com/dumbunny/vitess.git","ssh_url":"git@github.com:dumbunny/vitess.git","clone_url":"https://github.com/dumbunny/vitess.git","svn_url":"https://github.com/dumbunny/vitess","homepage":null,"size":50480,"stargazers_count":0,"watchers_count":0,"language":"Go","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"youtube:master","ref":"master","sha":"f80b78dd37c2ade572b5ee1c3ceaba7ad4ccab1e","user":{"login":"youtube","id":4052902,"avatar_url":"https://avatars.githubusercontent.com/u/4052902?v=3","gravatar_id":"","url":"https://api.github.com/users/youtube","html_url":"https://github.com/youtube","followers_url":"https://api.github.com/users/youtube/followers","following_url":"https://api.github.com/users/youtube/following{/other_user}","gists_url":"https://api.github.com/users/youtube/gists{/gist_id}","starred_url":"https://api.github.com/users/youtube/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/youtube/subscriptions","organizations_url":"https://api.github.com/users/youtube/orgs","repos_url":"https://api.github.com/users/youtube/repos","events_url":"https://api.github.com/users/youtube/events{/privacy}","received_events_url":"https://api.github.com/users/youtube/received_events","type":"Organization","site_admin":false},"repo":{"id":11008207,"name":"vitess","full_name":"youtube/vitess","owner":{"login":"youtube","id":4052902,"avatar_url":"https://avatars.githubusercontent.com/u/4052902?v=3","gravatar_id":"","url":"https://api.github.com/users/youtube","html_url":"https://github.com/youtube","followers_url":"https://api.github.com/users/youtube/followers","following_url":"https://api.github.com/users/youtube/following{/other_user}","gists_url":"https://api.github.com/users/youtube/gists{/gist_id}","starred_url":"https://api.github.com/users/youtube/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/youtube/subscriptions","organizations_url":"https://api.github.com/users/youtube/orgs","repos_url":"https://api.github.com/users/youtube/repos","events_url":"https://api.github.com/users/youtube/events{/privacy}","received_events_url":"https://api.github.com/users/youtube/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/youtube/vitess","description":"vitess provides servers and tools which facilitate scaling of MySQL databases for large scale web services.","fork":false,"url":"https://api.github.com/repos/youtube/vitess","forks_url":"https://api.github.com/repos/youtube/vitess/forks","keys_url":"https://api.github.com/repos/youtube/vitess/keys{/key_id}","collaborators_url":"https://api.github.com/repos/youtube/vitess/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/youtube/vitess/teams","hooks_url":"https://api.github.com/repos/youtube/vitess/hooks","issue_events_url":"https://api.github.com/repos/youtube/vitess/issues/events{/number}","events_url":"https://api.github.com/repos/youtube/vitess/events","assignees_url":"https://api.github.com/repos/youtube/vitess/assignees{/user}","branches_url":"https://api.github.com/repos/youtube/vitess/branches{/branch}","tags_url":"https://api.github.com/repos/youtube/vitess/tags","blobs_url":"https://api.github.com/repos/youtube/vitess/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/youtube/vitess/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/youtube/vitess/git/refs{/sha}","trees_url":"https://api.github.com/repos/youtube/vitess/git/trees{/sha}","statuses_url":"https://api.github.com/repos/youtube/vitess/statuses/{sha}","languages_url":"https://api.github.com/repos/youtube/vitess/languages","stargazers_url":"https://api.github.com/repos/youtube/vitess/stargazers","contributors_url":"https://api.github.com/repos/youtube/vitess/contributors","subscribers_url":"https://api.github.com/repos/youtube/vitess/subscribers","subscription_url":"https://api.github.com/repos/youtube/vitess/subscription","commits_url":"https://api.github.com/repos/youtube/vitess/commits{/sha}","git_commits_url":"https://api.github.com/repos/youtube/vitess/git/commits{/sha}","comments_url":"https://api.github.com/repos/youtube/vitess/comments{/number}","issue_comment_url":"https://api.github.com/repos/youtube/vitess/issues/comments{/number}","contents_url":"https://api.github.com/repos/youtube/vitess/contents/{+path}","compare_url":"https://api.github.com/repos/youtube/vitess/compare/{base}...{head}","merges_url":"https://api.github.com/repos/youtube/vitess/merges","archive_url":"https://api.github.com/repos/youtube/vitess/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/youtube/vitess/downloads","issues_url":"https://api.github.com/repos/youtube/vitess/issues{/number}","pulls_url":"https://api.github.com/repos/youtube/vitess/pulls{/number}","milestones_url":"https://api.github.com/repos/youtube/vitess/milestones{/number}","notifications_url":"https://api.github.com/repos/youtube/vitess/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/youtube/vitess/labels{/name}","releases_url":"https://api.github.com/repos/youtube/vitess/releases{/id}","created_at":"2013-06-27T21:20:28Z","updated_at":"2015-09-21T22:09:46Z","pushed_at":"2015-09-21T23:26:08Z","git_url":"git://github.com/youtube/vitess.git","ssh_url":"git@github.com:youtube/vitess.git","clone_url":"https://github.com/youtube/vitess.git","svn_url":"https://github.com/youtube/vitess","homepage":null,"size":72614,"stargazers_count":2805,"watchers_count":2805,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":false,"has_pages":true,"forks_count":356,"mirror_url":null,"open_issues_count":18,"forks":356,"open_issues":18,"watchers":2805,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/youtube/vitess/pulls/1120"},"html":{"href":"https://github.com/youtube/vitess/pull/1120"},"issue":{"href":"https://api.github.com/repos/youtube/vitess/issues/1120"},"comments":{"href":"https://api.github.com/repos/youtube/vitess/issues/1120/comments"},"review_comments":{"href":"https://api.github.com/repos/youtube/vitess/pulls/1120/comments"},"review_comment":{"href":"https://api.github.com/repos/youtube/vitess/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/youtube/vitess/pulls/1120/commits"},"statuses":{"href":"https://api.github.com/repos/youtube/vitess/statuses/3ba4d9d2e9609793c80b735256e74bca488c95ae"}}}}