Skip to content

Conversation

@sunjincheng121
Copy link
Member

What is the purpose of the change

(test case bug fix: add wait time for every record for testCancelSortMatchWhileDoingHeavySorting.)

Brief change log

  • add wait time for every record for testCancelSortMatchWhileDoingHeavySorting

Verifying this change

This change is a test case bug fix.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): ( no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: ( no )
  • The S3 file system connector: ( no)

Documentation

  • Does this pull request introduce a new feature? (no)

sunjincheng121 added a commit to sunjincheng121/flink that referenced this pull request Dec 17, 2018

private static final class SimpleMatcher<IN> implements JoinFunction<Tuple2<IN, IN>, Tuple2<IN, IN>, Tuple2<IN, IN>> {
private static final long serialVersionUID = 1L;
private static final int WAIT_TIME_PER_RECORD = 300; // 0.3 sec.
Copy link
Contributor

@hequn8128 hequn8128 Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunjincheng121 Thanks for reporting and fixing the problem. The test can pass with the fix. As for the changes, how about make delay configurable in DelayingMatcher? Keep SimpleMatcher as what it is, i.e, keep it simple.

Best, Hequn

Copy link
Member Author

@sunjincheng121 sunjincheng121 Dec 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Thanks for the review @hequn8128 !
If we using DelayingMatcher then we do not need this test case anymore. So I think both add the wait_time and make delayingMatcher are can not achieve the purpose of testing。 Because wait_time and delayingMatcher only test the join phase,we want test the sore-merge phase. So we should make sore-merge phase take more time to achieve the purpose of testing. I got a better approach,and will update the PR.

@sunjincheng121
Copy link
Member Author

Hi @hequn8128 I had update the PR. appreciate if you can look at it again!
Thanks,
Jincheng

@hequn8128
Copy link
Contributor

@sunjincheng121 Thanks for the update. The code is good. +1 to merge from my side.
Best, Hequj

sunjincheng121 added a commit to sunjincheng121/flink that referenced this pull request Dec 24, 2018
@sunjincheng121
Copy link
Member Author

Thanks @hequn8128, will be merged as soon as CI passed.

asfgit pushed a commit that referenced this pull request Dec 24, 2018
@asfgit asfgit closed this in 55c0b17 Dec 24, 2018
tisonkun pushed a commit to tisonkun/flink that referenced this pull request Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants