Page MenuHomePhabricator
Create Task
Maniphest T106596

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

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:

P1038 Gerrit trace for gerrit review --label verified=1
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

Details

SubjectRepoBranchLines +/-
Fix gerrit review case mismatch errors with labelsintegration/zuuldebian/precise-wikimedia+47 -0
Revert "Fix passing labels to Gerrit when they are not defined in All-Projects"integration/zuulpatch-queue/debian/precise-wikimedia+1 -1
Customize query in gerrit

Related Objects
Search...

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)
hashar added projects: Zuul, Continuous-Integration-Infrastructure, Gerrit.
hashar added subscribers: thcipriani, Krenair, Ricordisamoa and 2 others.

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/

hashar mentioned this in T106531: Changes voted CR+2 do not enter gate-and-submit unless Verified is +2..Jul 22 2015, 8:49 PM
hashar added a comment.Jul 22 2015, 8:57 PM

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

gerritbot subscribed.Jul 22 2015, 9:06 PM

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

gerritbot added a project: Patch-For-Review.Jul 22 2015, 9:06 PM
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.

hashar added a comment.Jul 23 2015, 12:17 PM

The case insensitive collation was introduced by https://gerrit.wikimedia.org/r/#/c/6439/1/templates/gerrit/gerrit.config.erb,unified for T37626: Extended characters show up as "?" in Gerrit user names

gerritbot added a comment.Jul 23 2015, 2:05 PM

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

hashar mentioned this in rCIZUc20985708764: Revert "Fix passing labels to Gerrit when they are not defined in All-Projects".Jul 23 2015, 2:08 PM
gerritbot added a comment.Jul 23 2015, 2:21 PM

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

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

gerritbot added a comment.Jul 23 2015, 3:18 PM

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

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

hashar mentioned this in rCIZU980c4422ae3a: Fix gerrit review case mismatch errors with labels.Jul 23 2015, 3:19 PM
hashar added a comment.Jun 2 2016, 12:29 PM

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.

hashar mentioned this in T152640: Cannot log into Gerrit as of recent upgrade.Jul 19 2017, 9:32 AM
• Phabricator_maintenance added a project: RelEng-Archive-FY201718-Q1.Dec 21 2017, 7:01 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits