Code review comment for lp:~tsarev/percona-server/5.1_bug_838725_fix

Revision history for this message
Oleg Tsarev (tsarev) wrote :

 > ** after your changes have_innodb_plugin.inc is functionally identical to have_innodb.inc. Which means there's no need for s/have_innodb/have_innodb_plugin/ in percona tests

ACK

 > ** mtr is just a symlink to mysql-test-run.pl, there's no need to duplicate diff changes

Unfortunatelly - not a symlink. You can check this by call "make all" and see to the Percona_Server-*-*/mysql-test/".

 > ** please revert the "Running test cases" changes. they might be good for debugging, but they don't carry any functional changes, so they can only increase maintenance cost.

This output very good for analyzing Jenkins logs. I don't see the big increase of maintenance time.
This is useful for me at least. Can we save this?

> ** MySQL bug #62340 reported by you is invalid. The difference in results has been introduced by your changes in Percona Server, it has nothing to do with upstream.

1) Bug #62340 is open yet. Where did you see this bug as "Invalid"?
2) Query " select text1, length(text1) from t1 where text1='teststring' or text1 like
'teststring_%';" has no guarantees for output order. I fix upstream test by "order by"
3) This changes to Percona-Server not is "my". This test doesn't work long time according to bug #838725, and right now start work again.

> ** main.index_merge_innodb, main.rowid_order_innodb, main.type_bit_innodb and main.endspace result differences look fishy. I understand that there may be a difference in results between builtin InnoDb and plugin/XtraDB. But those tests used to pass with XtraDB in older Percona Server releases. Isn't it something we should look into instead of just updating .result files? In other words, what's changed in those tests or XtraDB between Percona Server 5.1.55 and 5.1.58 that leads to results being different?

This is a good question, but I hasn't answer to it. May be ask Yasufumi for that? I will write letter to him.

> ** I would like a link to a Jenkins build on all slaves, not just the centos ones.

When all slaves will work fine, I will run tests on all platforms.

review: Needs Resubmitting

« Back to merge proposal