Page MenuHomePhabricator

gerrit review --label verified=1 causes a SQL error
Closed, DeclinedPublic

Description

Zuul has been changed the way it sends its code-review/verified votes. Instead of using --verified 2 it nows uses: --label verified=2.

But:

$ ssh -p 29418 hashar@gerrit.wikimedia.org 'gerrit review --label verified=1 226272,1'
fatal: internal server error while approving 226272,1
one or more approvals failed; review output above

Passing the label uppercase with --label Verified=-1 does work as expected.

On the server side with --label verified=-1 we have:

[2015-07-22 20:20:56,774] ERROR com.google.gerrit.sshd.commands.ReviewCommand : internal error while approving 226272,1
com.google.gwtorm.server.OrmException: insert failure on patch_set_approvals
	at com.google.gwtorm.schema.sql.SqlDialect.convertError(SqlDialect.java:152)
	...
Caused by: java.sql.BatchUpdateException: Duplicate entry '226272-1-24-Verified' for key 'PRIMARY'
	...
Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLIntegrityConstraintViolationException: Duplicate entry '226272-1-24-Verified' for key 'PRIMARY'

Full trace:

1[2015-07-22 20:20:56,774] ERROR com.google.gerrit.sshd.commands.ReviewCommand : internal error while approving 226272,1
2com.google.gwtorm.server.OrmException: insert failure on patch_set_approvals
3 at com.google.gwtorm.schema.sql.SqlDialect.convertError(SqlDialect.java:152)
4 at com.google.gwtorm.jdbc.JdbcAccess.convertError(JdbcAccess.java:448)
5 at com.google.gwtorm.jdbc.JdbcAccess.insert(JdbcAccess.java:160)
6 at com.google.gerrit.server.change.PostReview.updateLabels(PostReview.java:467)
7 at com.google.gerrit.server.change.PostReview.apply(PostReview.java:180)
8 at com.google.gerrit.sshd.commands.ReviewCommand.applyReview(ReviewCommand.java:217)
9 at com.google.gerrit.sshd.commands.ReviewCommand.approveOne(ReviewCommand.java:276)
10 at com.google.gerrit.sshd.commands.ReviewCommand.run(ReviewCommand.java:194)
11 at com.google.gerrit.sshd.SshCommand$1.run(SshCommand.java:35)
12 at com.google.gerrit.sshd.BaseCommand$TaskThunk.run(BaseCommand.java:442)
13 at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
14 at java.util.concurrent.FutureTask.run(FutureTask.java:262)
15 at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:178)
16 at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:292)
17 at com.google.gerrit.server.git.WorkQueue$Task.run(WorkQueue.java:360)
18 at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
19 at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
20 at java.lang.Thread.run(Thread.java:745)
21Caused by: java.sql.BatchUpdateException: Duplicate entry '226272-1-24-Verified' for key 'PRIMARY'
22 at com.mysql.jdbc.PreparedStatement.executeBatchSerially(PreparedStatement.java:2054)
23 at com.mysql.jdbc.PreparedStatement.executeBatch(PreparedStatement.java:1467)
24 at com.google.gwtorm.schema.sql.SqlDialect.executeBatch(SqlDialect.java:390)
25 at com.google.gwtorm.jdbc.JdbcAccess.execute(JdbcAccess.java:438)
26 at com.google.gwtorm.jdbc.JdbcAccess.insertAsBatch(JdbcAccess.java:202)
27 at com.google.gwtorm.jdbc.JdbcAccess.insert(JdbcAccess.java:155)
28 ... 15 more
29Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLIntegrityConstraintViolationException: Duplicate entry '226272-1-24-Verified' for key 'PRIMARY'
30 at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
31 at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
32 at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
33 at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
34 at com.mysql.jdbc.Util.handleNewInstance(Util.java:411)
35 at com.mysql.jdbc.Util.getInstance(Util.java:386)
36 at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:1040)
37 at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:4074)
38 at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:4006)
39 at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:2468)
40 at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:2629)
41 at com.mysql.jdbc.ConnectionImpl.execSQL(ConnectionImpl.java:2719)
42 at com.mysql.jdbc.PreparedStatement.executeInternal(PreparedStatement.java:2155)
43 at com.mysql.jdbc.PreparedStatement.executeUpdate(PreparedStatement.java:2450)
44 at com.mysql.jdbc.PreparedStatement.executeBatchSerially(PreparedStatement.java:2006)
45 ... 20 more

Obvious fix: use the upper case version :-D

Event Timeline

hashar created this task.Jul 22 2015, 8:30 PM
hashar closed this task as Declined.
hashar claimed this task.
hashar raised the priority of this task from to Needs Triage.
hashar updated the task description. (Show Details)

Filled it for history purpose. A potential fix would be to set the relevant database column to be case insensitive but that is probably to cause a bunch of mess.

hashar added a comment.EditedJul 22 2015, 8:47 PM

I checked on OpenStack and both works:

$ ssh -p 29418 review.openstack.org 'gerrit review --label code-review=-1 204753,1'
$

$ ssh -p 29418 hashar@review.openstack.org 'gerrit review --label code-review=0 204753,1'
$

$ ssh -p 29418 hashar@review.openstack.org 'gerrit review --label Code-Review=-1 204753,1'
$

The Zuul patch that switched to --label is https://review.openstack.org/#/c/138132/

The OpenStack Gerrit forks at https://review.openstack.org/p/openstack-infra/gerrit.git does not seem to have anything related:

git lg v2.8.4..openstack/openstack/2.8.4

* 4548330 - (openstack/openstack/2.8.4) Add username to stream-events queue entries (3 months ago) <James E. Blair>
* 384f3a6 - Revert "Add username to stream-events queue entries" (3 months ago) <James E. Blair>
* e391a2d - Add username to stream-events queue entries (4 months ago) <James E. Blair>
* 36a649d - Add change-owner parameter to gerrit hooks (1 year, 1 month ago) <Khai Do>
* 6dc8444 - Fix Schema_86 to use correct UUID for Change Owner group (1 year, 3 months ago) <Shawn Pearce>
* d584089 - add a new .gitreview file (1 year, 3 months ago) <Khai Do>
* 20aacd8 - Add a Change Owner group (1 year, 3 months ago) <Khai Do>
* 8483f17 - replace submodule url reference from relative to absolute (1 year, 3 months ago) <Khai Do>
* 977072b - Fix: Wrong exception mapping in ReceiveCommmits (1 year, 3 months ago) <Bruce Zu>
* 4b77e60 - Fix REST example for removing included groups from a group (1 year, 3 months ago) <Edwin Kempin>
* d2b9217 - rest-api-groups.txt: Correct input examples to use [] for lists (1 year, 3 months ago) <Dave Borowitz>
* 77bb620 - Enable automatic close changes on 'refs/meta/config' (1 year, 3 months ago) <Bruce Zu>
* d5edded - Implement pagination in group list screen (1 year, 3 months ago) <Anthony Chin>
* dc78329 - Add option 'n' and 'S' to groups REST API to limit group results. (1 year, 3 months ago) <Anthony Chin>
* e3ba9e4 - Do not refresh group list if filter did not change (1 year, 3 months ago) <Anthony Chin>
* 6534b9e - Don't show the submit button for draft patch sets (1 year, 3 months ago) <David Pursehouse>
* 62096c7 - Prevent draft changes from being abandoned (1 year, 3 months ago) <David Ostrovsky>
* 953784a - Ensure NrtFutures are notified promptly of new search generation (1 year, 3 months ago) <Dave Borowitz>
* 8fdf043 - Fix deadlocks in SubIndex shutdown (1 year, 3 months ago) <Dave Borowitz>

James E. Blair confirmed: ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin

That is a mystery :-) gotta fix zuul

Change 226415 had a related patch set uploaded (by Hashar):
Revert "Fix passing labels to Gerrit when they are not defined in All-Projects"

https://gerrit.wikimedia.org/r/226415

hashar added a comment.EditedJul 23 2015, 12:07 PM

On the Gerrit database:

select * from patch_set_approvals where change_id=226272 ;

valuegrantedchange_openchange_sort_keychange_idpatch_set_idaccount_idcategory_id
-12015-07-22 20:23:04N00369eef000373e0226272124Verified
 show create table patch_set_approvals \G
*************************** 1. row ***************************
       Table: patch_set_approvals
Create Table: CREATE TABLE `patch_set_approvals` (
  `value` smallint(6) NOT NULL DEFAULT '0',
  `granted` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  `change_open` char(1) NOT NULL DEFAULT 'N',
  `change_sort_key` varchar(16) DEFAULT NULL,
  `change_id` int(11) NOT NULL DEFAULT '0',
  `patch_set_id` int(11) NOT NULL DEFAULT '0',
  `account_id` int(11) NOT NULL DEFAULT '0',
  `category_id` varchar(255) CHARACTER SET utf8 COLLATE utf8_bin NOT NULL DEFAULT '',
  PRIMARY KEY (`change_id`,`patch_set_id`,`account_id`,`category_id`),
  KEY `patch_set_approvals_openByUser` (`change_open`,`account_id`),
  KEY `patch_set_approvals_closedByU` (`change_open`,`account_id`,`change_sort_key`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8
1 row in set (0.00 sec)

Our Gerrit MySQL Connection has: connectionCollation=utf8_unicode_ci where ci stands for case insensitive. OpenStack does not seem to have it.

Change 226415 merged by Hashar:
Revert "Fix passing labels to Gerrit when they are not defined in All-Projects"

https://gerrit.wikimedia.org/r/226415

Change 226532 had a related patch set uploaded (by Hashar):
Fix gerrit review case mismatch errors with labels

https://gerrit.wikimedia.org/r/226532

Change 226532 merged by Hashar:
Fix gerrit review case mismatch errors with labels

https://gerrit.wikimedia.org/r/226532

I had to include a revert patch in our Zuul deployment. As of 2.1.0-151-g30a433b-wmf1precise1 it is no more needed since upstream rewrote a large part of how Zuul interact with Zuul and it is not affected anymore.