Skip to content

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Oct 28, 2015

This PR removes all dependencies to the Record API in the flink-tests project.

For this, tests were either ported or removed.

The following files were NOT ported:

  • recordJobs/*
    • except kmeans/udfs CoordVector, PointInFormat (moved & ported)
    • except util/InfiniteIntegerInputFormat (moved & ported)
  • recordJobTests/*
  • operators/*
    • except CoGroupSortITCase, MapPartitionITCase, ObjectReuseITCase (moved)
  • accumulators/AccumulatorIterativeITCase
    • Unfinished test, should be addressed in a separate issue
  • IterationTerminationWithTwoTails
    • nigh identical with IterationTerminationWithTerminationTail (ported version also failed)
  • iterative/DeltaPageRankITCase, IterativeKMeansITCase, KMeansITCase
    • behaved like they belong into /recordJobTests/ (as in they simply execute a recordJob), thus removed
  • optimizer/iterations/IterativeKMeansITCase
    • overlaps with ConnectedComponentsITCase (need a second opinion on this!)
  • util/FailingTestBase
    • integrated into TaskFailureITCase (only user of the class)
  • testPrograms/util/tests/*

cancelling/MatchJoinCancellingITCase was ported but disabled since it is unstable, and also disabled before. Separate issue seems appropriate.

@fhueske
Copy link
Contributor

fhueske commented Oct 28, 2015

Nice @zentol! Thanks for this effort!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this test and port it.

@fhueske
Copy link
Contributor

fhueske commented Nov 10, 2015

Hi @zentol, thanks again for the huge effort to port all these tests!

Looks mostly good, except for some minor things and three tests which should be ported to preserve test coverage, IMO.

@zentol
Copy link
Contributor Author

zentol commented Nov 17, 2015

@fhueske I've addressed most of your concerns.

Things that still need work / clarification:

  • PreviewPlanDumpTest was previously executed with 2 different sets of arguments, now only with 1. Should this be changed back to the previous behaviour? The arguments affect paths for sources/sink, parallelism and the number of Iterations
  • test.classloading.jar.KMeansForTest appears to be a good replacement for the IterativeKMeansITCase, what's your take on that?
  • The removed delta ilteration PageRank program looks very similar to the ConnectedComponents implementation under flink-examples. I don't think this needs a separate port.

@fhueske
Copy link
Contributor

fhueske commented Nov 24, 2015

Hi @zentol, sorry for the delayed review.
You are right, DeltaPageRankITCase (which executes for some reason the Record API delta iteration ConnectedComponents example) is identical to the DataSet ConnectedComponentsITCase. So we do not port this one.
I also agree to remove the checks for empty arguments in the PreviewPlanDumpTest. This requires cooperation of the job, i.e., the job may not fail for empty arguments, and tests exactly the same code paths as the tests with arguments, as far as I see.

However, I do not think that the ClassLoaderITCase which calls the KMeansForTest is a good replacement for the IterativeKMeansITCase. The programs are identical but, the ClassLoaderITCase only checks if the program executes without an error and does not check the result.

@zentol Is it OK for you if I add this one last test and merge this PR afterwards?

@zentol
Copy link
Contributor Author

zentol commented Nov 24, 2015

Go ahead, i won't have time for it today anyway :)

@fhueske
Copy link
Contributor

fhueske commented Nov 24, 2015

Thanks, I'll port the last test and merge this PR

fhueske pushed a commit to fhueske/flink that referenced this pull request Nov 24, 2015
@asfgit asfgit closed this in ebba20d Nov 24, 2015
@zentol zentol deleted the 2901_record_tests branch November 24, 2015 20:59
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.

4 participants