Skip to content

[HUDI-6076] Fix the unit of clustering.plan.strategy.target.file.max.bytes for clustering#8455

Merged
danny0405 merged 3 commits intoapache:masterfrom
Zouxxyy:xinyu/fix-size
Apr 18, 2023
Merged

[HUDI-6076] Fix the unit of clustering.plan.strategy.target.file.max.bytes for clustering#8455
danny0405 merged 3 commits intoapache:masterfrom
Zouxxyy:xinyu/fix-size

Conversation

@Zouxxyy
Copy link
Contributor

@Zouxxyy Zouxxyy commented Apr 14, 2023

Change Logs

clustering.plan.strategy.target.file.max.bytes's unit is byte, clustering.plan.strategy.small.file.limit should be unified with it. And they also compare sizes somewhere, there's not even a uniform unit here, like this

  this.conf.setLong(FlinkOptions.CLUSTERING_PLAN_STRATEGY_SMALL_FILE_LIMIT.key(),
      this.conf.getLong(FlinkOptions.CLUSTERING_PLAN_STRATEGY_TARGET_FILE_MAX_BYTES) > this.conf.getLong(FlinkOptions.CLUSTERING_PLAN_STRATEGY_SMALL_FILE_LIMIT)
        ? this.conf.getLong(FlinkOptions.CLUSTERING_PLAN_STRATEGY_SMALL_FILE_LIMIT)
          : this.conf.getLong(FlinkOptions.CLUSTERING_PLAN_STRATEGY_TARGET_FILE_MAX_BYTES));

Impact

Fix the unit of clustering.plan.strategy.target.file.max.bytes for clustering

Risk level (write none, low medium or high below)

low

Documentation Update

none

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Apr 14, 2023

@danny0405 Can you help with a review?

@SteNicholas
Copy link
Member

SteNicholas commented Apr 14, 2023

@Zouxxyy, did you take the compatibility of this change into consideration? With this change, the config value of clustering.plan.strategy.small.file.limit must be changed when upgrade to the lastest version. IMO, you should introduce new option for the config unification.

@danny0405
Copy link
Contributor

@Zouxxyy, did you take the compatibility of this change into consideration? With this change, the config value of clustering.plan.strategy.small.file.limit must be changed when upgrade to the lastest version. IMO, you should introduce new option for the config unification.

A valid concern, this may break the user code, we should take the modification as early and add some illustrations on the release note.

@danny0405 danny0405 self-assigned this Apr 15, 2023
@danny0405 danny0405 added engine:flink Flink integration configs labels Apr 15, 2023
@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Apr 15, 2023

A valid concern, this may break the user code, we should take the modification as early and add some illustrations on the release note.

@SteNicholas @danny0405 So should we add new configuration, or just fix the bug and not change the unit?

@danny0405
Copy link
Contributor

just fix the bug and not change the unit

Let's fix the bug first.

@Zouxxyy
Copy link
Contributor Author

Zouxxyy commented Apr 17, 2023

@danny0405 done

@danny0405 danny0405 changed the title [HUDI-6076] Change clustering.plan.strategy.small.file.limit's unit to byte [HUDI-6076] Fix the unit of clustering.plan.strategy.target.file.max.bytes for clustering Apr 17, 2023
@hudi-bot
Copy link
Collaborator

CI report:

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

@danny0405 danny0405 merged commit 2c0a5f4 into apache:master Apr 18, 2023
stayrascal pushed a commit to stayrascal/hudi that referenced this pull request Apr 20, 2023
yihua pushed a commit to yihua/hudi that referenced this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine:flink Flink integration

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants