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-10569] Remove Instance usage in FailoverRegionTest #7722

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Feb 17, 2019

What is the purpose of the change

Temporarily, use SimpleSlotProvider to get rid of usage of Instance. However, SimpleSlot is a legacy resource concept. A follow up should refactor SimpleSlotProvider to TestingLogicalSlotProvider or something.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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)

cc @tillrohrmann

@flinkbot
Copy link
Collaborator

flinkbot commented Feb 17, 2019

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.

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.

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot approve description to approve the 1st aspect (similarly, it also supports the consensus, architecture and quality keywords)
  • @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

@zentol zentol self-assigned this Feb 20, 2019
@zentol
Copy link
Contributor

zentol commented Feb 20, 2019

Why can't we refactor the test to TestingLogicalSlotProvider now? What is blocking this effort?

@tisonkun
Copy link
Member Author

@zentol Actually I did the refactor. The only possibly problematic change is I expose AllocatedSlot to public. Since it has a public constructor I guess it could be OK. If not so, we are going to add a testing SlotContext with similar functions as AllocatedSlot.

@tisonkun tisonkun force-pushed the FLINK-10569-FailoverRegionTest branch 2 times, most recently from 0fa4fd7 to 3e29229 Compare February 21, 2019 02:39
@zentol
Copy link
Contributor

zentol commented Feb 21, 2019

oooooki now that I see it this TestingLogicalSlotProvider business is out-of-scope for this PR. Please open a separate JIRA for that.
I will merge the first commit though.

@tisonkun
Copy link
Member Author

OK I file a JIRA FLINK-11710. Is there anything I should do with thi pr? In my understanding we merge the first commit and close this pr. The second commit will be in another pr.

@zentol
Copy link
Contributor

zentol commented Feb 21, 2019

you don't have to do anything to this PR, I'll take care of it.

@zentol zentol force-pushed the FLINK-10569-FailoverRegionTest branch from 3e29229 to ae899f5 Compare February 21, 2019 11:57
@zentol zentol merged commit 8bf043a into apache:master Feb 22, 2019
@tisonkun tisonkun deleted the FLINK-10569-FailoverRegionTest branch February 22, 2019 11:04
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