Hi,
I am trying make small review for slow test and I found small issue: NonReusingReOpenableHashTableITCase testSpillingHashJoinWithMassiveCollisions testSpillingHashJoinWithTwoRecursions for testSpillingHashJoinWithTwoRecursions exist description /* * This test is basically identical to the "testSpillingHashJoinWithMassiveCollisions" test, only that the number * of repeated values (causing bucket collisions) are large enough to make sure that their target partition no longer * fits into memory by itself and needs to be repartitioned in the recursion again. */ but he incorrect, because code of both test fully equal, one difference line very similar on bug after refactoring with inserting recordReuse testSpillingHashJoinWithMassiveCollisions 353 while ((record = buildSide.next(record)) != null) { (f51f1b4 19.03.14, 1:17 Aljoscha Krettek* Change MutableObjectIterator to allow immutable objects) Aljoscha, can we remove one test and fix buildSide.next(record) to buildSide.next(recordReuse) ? P.S. I started review because we have a lot of failing test due to cpu time limit Thanks, Alexey |
Thanks for looking into this! I think we can put in the fix and remove one
of the tests, yes. @Robert What do you think? I think you initially added this test a loooong while back. On Mon, 9 Jan 2017 at 20:11 Alexey Demin <[hidden email]> wrote: > Hi, > > I am trying make small review for slow test and I found small issue: > > NonReusingReOpenableHashTableITCase > > testSpillingHashJoinWithMassiveCollisions > testSpillingHashJoinWithTwoRecursions > > > for testSpillingHashJoinWithTwoRecursions exist description > > /* > * This test is basically identical to the > "testSpillingHashJoinWithMassiveCollisions" test, only that the number > * of repeated values (causing bucket collisions) are large enough to make > sure that their target partition no longer > * fits into memory by itself and needs to be repartitioned in the > recursion again. > */ > > but he incorrect, because code of both test fully equal, > one difference line very similar on bug after refactoring with inserting > recordReuse > > testSpillingHashJoinWithMassiveCollisions > 353 while ((record = buildSide.next(record)) != null) { > > (f51f1b4 19.03.14, 1:17 Aljoscha Krettek* Change MutableObjectIterator to > allow immutable objects) > > > Aljoscha, can we remove one test and fix buildSide.next(record) to > buildSide.next(recordReuse) ? > > P.S. I started review because we have a lot of failing test due to cpu > time limit > > Thanks, > Alexey > |
I think the code has been refactored many times since then.
If the code of the tests is really the same I'm okay with deleting the duplicate method. On Tue, Jan 10, 2017 at 3:29 PM, Aljoscha Krettek <[hidden email]> wrote: > Thanks for looking into this! I think we can put in the fix and remove one > of the tests, yes. > > @Robert What do you think? I think you initially added this test a loooong > while back. > > On Mon, 9 Jan 2017 at 20:11 Alexey Demin <[hidden email]> wrote: > > > Hi, > > > > I am trying make small review for slow test and I found small issue: > > > > NonReusingReOpenableHashTableITCase > > > > testSpillingHashJoinWithMassiveCollisions > > testSpillingHashJoinWithTwoRecursions > > > > > > for testSpillingHashJoinWithTwoRecursions exist description > > > > /* > > * This test is basically identical to the > > "testSpillingHashJoinWithMassiveCollisions" test, only that the number > > * of repeated values (causing bucket collisions) are large enough to > make > > sure that their target partition no longer > > * fits into memory by itself and needs to be repartitioned in the > > recursion again. > > */ > > > > but he incorrect, because code of both test fully equal, > > one difference line very similar on bug after refactoring with inserting > > recordReuse > > > > testSpillingHashJoinWithMassiveCollisions > > 353 while ((record = buildSide.next(record)) != null) { > > > > (f51f1b4 19.03.14, 1:17 Aljoscha Krettek* Change MutableObjectIterator to > > allow immutable objects) > > > > > > Aljoscha, can we remove one test and fix buildSide.next(record) to > > buildSide.next(recordReuse) ? > > > > P.S. I started review because we have a lot of failing test due to cpu > > time limit > > > > Thanks, > > Alexey > > > |
Hi
https://github.com/apache/flink/pull/3089 Check please 2017-01-10 18:42 GMT+04:00 Robert Metzger <[hidden email]>: > I think the code has been refactored many times since then. > > If the code of the tests is really the same I'm okay with deleting the > duplicate method. > > On Tue, Jan 10, 2017 at 3:29 PM, Aljoscha Krettek <[hidden email]> > wrote: > >> Thanks for looking into this! I think we can put in the fix and remove one >> of the tests, yes. >> >> @Robert What do you think? I think you initially added this test a loooong >> while back. >> >> On Mon, 9 Jan 2017 at 20:11 Alexey Demin <[hidden email]> wrote: >> >> > Hi, >> > >> > I am trying make small review for slow test and I found small issue: >> > >> > NonReusingReOpenableHashTableITCase >> > >> > testSpillingHashJoinWithMassiveCollisions >> > testSpillingHashJoinWithTwoRecursions >> > >> > >> > for testSpillingHashJoinWithTwoRecursions exist description >> > >> > /* >> > * This test is basically identical to the >> > "testSpillingHashJoinWithMassiveCollisions" test, only that the number >> > * of repeated values (causing bucket collisions) are large enough to >> make >> > sure that their target partition no longer >> > * fits into memory by itself and needs to be repartitioned in the >> > recursion again. >> > */ >> > >> > but he incorrect, because code of both test fully equal, >> > one difference line very similar on bug after refactoring with inserting >> > recordReuse >> > >> > testSpillingHashJoinWithMassiveCollisions >> > 353 while ((record = buildSide.next(record)) != null) { >> > >> > (f51f1b4 19.03.14, 1:17 Aljoscha Krettek* Change MutableObjectIterator >> to >> > allow immutable objects) >> > >> > >> > Aljoscha, can we remove one test and fix buildSide.next(record) to >> > buildSide.next(recordReuse) ? >> > >> > P.S. I started review because we have a lot of failing test due to cpu >> > time limit >> > >> > Thanks, >> > Alexey >> > >> > > |
Free forum by Nabble | Edit this page |