Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-10778] [tests] Extend TypeSerializerSnapshotMigrationTestBase to test migrating from multiple versions #7504

Closed
wants to merge 17 commits into from

Conversation

tzulitai
Copy link
Contributor

What is the purpose of the change

Prior to this PR, the TypeSerializerSnapshotMigrationTestBase was hardcoded to only test restoring from serializer snapshots generated in Flink 1.6.x.

The main goal of this PR is to:

  • Make the test base version-aware so that multiple serializer snapshots can be tested
  • Extend all existing subclasses to test migrating from a 1.7.x snapshot (test snapshots are generated and added to this PR)
  • There are some other minor changes to the test base, to better match the actual usage / API contracts of the TypeSerializerSnapshot abstraction.

Brief change log

  • 8303eec: Move MigrationVersion class to flink-core, since it'll be used by TypeSerializerSnapshotMigrationTestBase.
  • f5b587a: Makes TypeSerializerSnapshotMigrationTestBase version-aware. Test specifications can now define the version that the target test snapshot was taken in.
  • a13ae99 to 2196635: Some minor changes to the test base to better match usage & API contracts of the TypeSerializerSnapshot abstraction.
  • 3ba6a5e: Adds a utility TestSpecifications class that would help with de-duplicating code when specifying test specifications for multiple versions.
  • c5d20b7 to 200edc7: Adding MigrationVersion.1.7 to the parameters of all existing TypeSerializerSnapshotMigrationTestBase subclasses, as well as generating the required test snapshot / data files for them.

Verifying this change

This is a refactoring of tests.
All subclasses of TypeSerializerSnapshotMigrationTestBase should be passing still.

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
  • If yes, how is the feature documented? not applicable

That utility class had a single helper method, restoreFromSnapshot,
which accepts the target snapshot's Flink version. This was useful
before, because the way to restore snapshots was a bit different for
Flink <= 1.1 and newer versions.

Since we now no longer support compatibility for 1.1 versions and
below, this helper method is simply forwarding the restore operation
to the test harness.

This commit refactors this have equivalent behaviour directly in the
AbstractStreamOperatorTestHarness class.
@tzulitai tzulitai force-pushed the FLINK-10778 branch 2 times, most recently from 41a547f to 7f9c4f1 Compare January 17, 2019 09:02
…are of snapshot version

Before, the tests in TypeSerializerSnapshotMigrationTestBase always
assumed that the test serializer snapshotw were written with Flink 1.6.

This commit makes this flexible, so that we can allow subclasses to
specify different snapshot versions for each TestSpecification.
…exible in TypeSerializerSnapshotMigrationTestBase

Before, the TypeSerializerSnapshotMigrationTestBase test base always
only asserted that a new serializer is compatible as is with the
previous serializer's snapshot.

This makes the test code not resusable for cases where the new
serializer provided isn't compatible as is, but other compatibility
types, for example Kryo serializers that requires reconfiguration on
restore. This allows the test base to express those cases.
…zer test in TypeSerializerSnapshotMigrationTestBase

The serializerSnapshotRestoresCurrentSerializer test asserts that the
restored serializer instance from the snapshot is of the same type as
the current serializer. This isn't necessary true, as is not a contract
of the TypeSerializerSnapshot abstraction.

Therefore, this test is removed.
…ild test specifications for multiple test versions
…to test restoring from 1.7

This commit also refactors the BaseTypeSerializerSnapshotMigrationTest
by using the new TypeSerializations utility class, to reduce the amount
of duplicate code.
…nTest to test restoring from 1.7

This commit also refactors the
PrimitiveArraySerializerSnapshotMigrationTest by using
the new TestSerializations utility class, to reduce the
amount of duplicate code.
@tzulitai
Copy link
Contributor Author

Since Travis is green and we've already went through reviews offline with @igalshilman, will proceed to merge this ..

@asfgit asfgit closed this in 76dd766 Jan 17, 2019
tisonkun pushed a commit to tisonkun/flink that referenced this pull request Jan 17, 2019
…ase subclasses to test restoring from 1.7

This closes apache#7504.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants