Slow duplicated tests

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Slow duplicated tests

Alexey Demin
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
Reply | Threaded
Open this post in threaded view
|

Re: Slow duplicated tests

Aljoscha Krettek-2
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
>
Reply | Threaded
Open this post in threaded view
|

Re: Slow duplicated tests

Robert Metzger
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
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Slow duplicated tests

Alexey Demin
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
>> >
>>
>
>