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-32809][yarn] Fixes YarnClusterDescriptor#isArchiveOnlyIncluded… #23191

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

qinf
Copy link
Contributor

@qinf qinf commented Aug 10, 2023

…InShipArchiveFiles dose not work as expected

What is the purpose of the change

(For example: This pull request makes task deployment go through the blob server, rather than through RPC. That way we avoid re-transferring them on each deployment (during recovery).)

Brief change log

  • Fixed YarnClusterDescriptor#isArchiveonlyincludedinShipArchiveFiles dose not work as expected

Verifying this change

This change added tests and can be verified as follows:

  • *Added test that validates that YarnConfigOptions.SHIP_ARCHIVES only support archive files *

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, Kubernetes/Yarn, 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 / docs / JavaDocs / not documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Aug 10, 2023

CI report:

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

@qinf
Copy link
Contributor Author

qinf commented Aug 10, 2023

Hi @1996fanrui would you mind helping review this PR in your free time?
It's a minor change, it just fixed YarnClusterDescriptor#isArchiveOnlyIncludedInShipArchiveFiles does not work as expected.

@1996fanrui 1996fanrui self-requested a review August 10, 2023 08:08
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Hi @qinf , thanks for your fix.

Overall is good, I have left a few comments, please take a look, thanks~

@1996fanrui
Copy link
Member

Hi @qinf , could you update the commit message to [FLINK-32809][yarn] Fix the bug that there is no check when yarn.ship-archives configures the directory?

It's more specific, and everyone can see what doesn't meet expectations.

Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Thanks @qinf for the quick update!

The change LGTM!

Hi @RocMarshal , would you mind helping take a look this PR in your free time as well? thanks~

Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

Thanks @qinf for the contribution and @1996fanrui for the review.
Looks good to me on the whole just left a few of comment.
PTAL in your free time.

Comment on lines +316 to +317
.map(File::getName)
.map(String::toLowerCase)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these two lines be inlined?
Because there's no filters between the two maps

Comment on lines +320 to +325
name.endsWith(".tar.gz")
|| name.endsWith(".tar")
|| name.endsWith(".tgz")
|| name.endsWith(".dst")
|| name.endsWith(".jar")
|| name.endsWith(".zip"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a possibility of frequent changes in the suffix format here?
If so, would you consider introducing a parameter to specify the suffix format?

Please correct me if I'm wrong in my limited read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RocMarshal The archive types have not changed for over 2 years, maybe it is a good time to do this when there is a new archive type need to add here.

Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

LGTM +1. Thanks @qinf
CC @1996fanrui

@1996fanrui
Copy link
Member

Thanks @RocMarshal for the review, merging~

@1996fanrui 1996fanrui merged commit ac85945 into apache:master Aug 11, 2023
RocMarshal pushed a commit to RocMarshal/flink that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants