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-20580][core] Don't support nullable value for SerializedValue #14936

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Feb 13, 2021

What is the purpose of the change

Dropping nullable-value support from SerializedValue to simplify its usages. Callers should resort to nullabe/optional variable of SerializedValue if them want such behavior but just can't treat SerializedValue as another optional container anymore.

Brief change log

  • Migrate usage of SerializedValue in rpc module to its own wire value class. This make this module self-contained and resistant to rpc usages of SerializedValue.
  • Dropping nullable-value support from SerializedValue.

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

  • SerializedValueTest.testSimpleValue.

This change added tests and can be verified as follows:

  • SerializedValueTest.testNullValue, SerializedValueTest.testFromNullBytes, SerializedValueTest.testFromEmptyBytes
  • AkkaRpcSerializedValueTest
  • AkkaRpcActorTest.canRespondWithSerializedValueLocally, RemoteAkkaRpcActorTest.canRespondWithSerializedValueRemotely

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: (don't know)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/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)

WARNING

This change will breaking tm/jm rolling update if we ever support it. I got nothing of such supporting from docs and assume no such guarantee.

@flinkbot
Copy link
Collaborator

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 3687cf9 (Sat Feb 13 05:43:59 UTC 2021)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@kezhuw kezhuw force-pushed the flink-20580-serialized-value-not-nullable-value branch from 3687cf9 to 1a04c87 Compare February 13, 2021 05:44
@flinkbot
Copy link
Collaborator

flinkbot commented Feb 13, 2021

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run travis re-run the last Travis build
  • @flinkbot run azure re-run the last Azure build

@kezhuw kezhuw force-pushed the flink-20580-serialized-value-not-nullable-value branch from 4895ac2 to 69e5215 Compare February 13, 2021 08:15
@kezhuw
Copy link
Member Author

kezhuw commented Feb 13, 2021

Azure FAILURE is unrelated to changes and reported to existing jira https://issues.apache.org/jira/browse/FLINK-21214.

@kezhuw
Copy link
Member Author

kezhuw commented Feb 13, 2021

@flinkbot run azure

@kezhuw
Copy link
Member Author

kezhuw commented Feb 13, 2021

Hi @rmetzger, @flinkbot run azure does not function anymore ?

@kezhuw kezhuw force-pushed the flink-20580-serialized-value-not-nullable-value branch from 69e5215 to f0b807f Compare February 13, 2021 13:29
@kezhuw
Copy link
Member Author

kezhuw commented Feb 15, 2021

@flinkbot attention @tillrohrmann

@rmetzger
Copy link
Contributor

The most reliable way of triggering another Azure build is by pushing to the branch again. It seemed to have worked this time.
I'll ask Chesnay to look into the reason why the command didn't work.

@kezhuw kezhuw force-pushed the flink-20580-serialized-value-not-nullable-value branch from f0b807f to ddb9314 Compare February 16, 2021 16:51
@tillrohrmann tillrohrmann self-assigned this Feb 17, 2021
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Very good work @kezhuw! The changes look good to me. I will address my own comments while merging this PR.

SerializedValue<String> saved =
SerializedValue.fromBytes(Arrays.copyOf(bytes, bytes.length));
assertEquals(v, saved);
assertArrayEquals(v.getByteArray(), saved.getByteArray());
} catch (Exception e) {
e.printStackTrace();
fail(e.getMessage());
}
}

@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a bit shorter:

Suggested change
@Test
@Test(expected = NullPointerException.class)


assertNull(copy.deserializeValue(getClass().getClassLoader()));
@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with @Test(expected = ...).

} catch (Exception e) {
e.printStackTrace();
fail(e.getMessage());
@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with @Test(exepcted = ...).

import static org.hamcrest.Matchers.nullValue;
import static org.junit.Assert.assertThat;

public class AkkaRpcSerializedValueTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public class AkkaRpcSerializedValueTest {
public class AkkaRpcSerializedValueTest extends TestLogger {

This makes sure that the test names are printed. This is quite helpful for CI where multiple tests are executed one after another and the names help to split the log messages from the different test cases.

@kezhuw
Copy link
Member Author

kezhuw commented Feb 18, 2021

@tillrohrmann Thanks for reviewing, the comments make sense to me. Then I will give up rebasing to you.

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