Skip to content

Core: Change removal of deprecations to 1.12.0 - #14392

Merged
pvary merged 1 commit into
apache:mainfrom
gaborkaszab:main_change_deprecations_core
Dec 15, 2025
Merged

Core: Change removal of deprecations to 1.12.0#14392
pvary merged 1 commit into
apache:mainfrom
gaborkaszab:main_change_deprecations_core

Conversation

@gaborkaszab

Copy link
Copy Markdown
Contributor

No description provided.

Comment threadcore/src/main/java/org/apache/iceberg/TableProperties.java Outdated

/**
* @deprecated will be removed in 2.0.0. Use {@link
* @deprecated will be removed in 1.12.0. Use {@link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this one should be fine to remove before 2.0.0, since the property was only moved to a different class

Comment threadcore/src/main/java/org/apache/iceberg/util/ThreadPools.java Outdated
/**
* @deprecated Use {@link SystemConfigs#WORKER_THREAD_POOL_SIZE WORKER_THREAD_POOL_SIZE} instead;
* will be removed in 2.0.0
* will be removed in 1.12.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be ok to remove earlier as well

@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@pvary

Copy link
Copy Markdown
Contributor

Limit this to the ones which are trivial, and merge those

@gaborkaszab
gaborkaszabforce-pushed the main_change_deprecations_core branch from c05ed54 to 398b988CompareDecember 1, 2025 08:51
@gaborkaszab

Copy link
Copy Markdown
ContributorAuthor

Thank you for taking a look @nastra and @pvary !
I remove the changes from TableProperties. Do you think the rest is reasonable?

@gaborkaszab
gaborkaszabforce-pushed the main_change_deprecations_core branch from 398b988 to 82673e6CompareDecember 1, 2025 09:12

/**
* @deprecated will be removed in 2.0.
* @deprecated will be removed in 1.12.0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CC: @ggershinsky - is there any particular reason to keep these to 2.0?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be ok to remove earlier.

*
* @param props table configuration
* @deprecated use {@link MetricsConfig#forTable(Table)}
* @deprecated use {@link MetricsConfig#forTable(Table)}. Will be removed in 1.12.0

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Change back to 2.0.0


/**
* @deprecated since 1.7.0, will be removed in 2.0.0; This method does only best-effort
* @deprecated since 1.7.0, will be removed in 1.12.0; This method does only best-effort

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

What is an alternative way to empty the cache?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@findepi Do you know any alternative way to invalidate all entries in the cache?

@pvary we discussed that we may want to leave the targeted removal for 2.0.0. However, this is a functionality we'd like the users to come off (due to the race condition) I think we should remove asap, so 1.12.0 still makes sense to me. WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know any alternative way to invalidate all entries in the cache?

Depends on the goal.
if you invalidate just to reduce memory usage (rare), this method is fine
if you invalidate because something changed and all values needs to be recomputed (including those in flight), then there is no ready to use solution.

In trino we have additional layer on top of Guava caches that solves this problem. Here we use caffeine, but last time i checked, it had the same invalidation race problem and the author's feedback was that it was fundamentally not solvable.
thus, if full invalidation is needed, you can either copy code from trino-cache trino module (or use the code i already extracted https://github.com/findepi/evictable-cache)

Here i'd hope that invalidation isn't really needed for anything and we can remove the API.


/**
* @deprecated will be removed in 2.0.0, use {@link ViewBuilder#withLocation} instead.
* @deprecated will be removed in 1.12.0, use {@link ViewBuilder#withLocation} instead.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

set back to 2.0.0

}

@Deprecated(since = "1.2")
/* @deprecated Will be removed in 1.12.0 */

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

check if tests pass without this

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think we can remove this once the PreparedStatement.setUnicodeStream is removed in JDK. It's out of our control, so I reset this change to original

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest to use @SuppressWarnings("deprecation") to get rid of the warning. Having @Deprecated(since = "1.2") is misleading. One might think that it is referring to the Iceberg 1.2 release


/**
* @deprecated will be removed in 2.0.0; use applyNameMapping and pruneColumns(Schema, Set)
* @deprecated will be removed in 1.12.0; use applyNameMapping and pruneColumns(Schema, Set)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

did the deprecation removal rules change?
it looks like we considered not ok to remove a public API in a minor update, and now we conclude it's ok.

has this been discussed somewhere already?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

There was a discussion about this in one of the community syncs. The conclusion was that we give guarantees for a whole major version in the api/ module, however in the core/ module we could be less strict and provide guarantees for one minor release. Technically deprecate in the upcoming minor, drop in the one after that is OK in core. Hence, this PR collects all the deprecations in core/ marking removal for 2.0.0 to revisit them and drop earlier if possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

however in the core/ module we could be less strict and provide guarantees for one minor release

is this written down somewhere so that we can reference the rules of the games when deprecating?
community syncs meeting notes are not normative, we should capture important conclusions somewhere

btw, i don't think this 1 minor release guarantee means much in practice. maven world reality is that minor version update may just happen when combining various artifacts for the final delivery, so removing an API may obviously lead to MethodDefNotFoundError. This may be a fair trade off, i just hope it's a conscious one

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks for taking a look, @findepi !
Here are the docs for the api guarantees in different modules. It mentions one minor version guarantee in core module.


/**
* @deprecated since 1.7.0, will be removed in 2.0.0; This method does only best-effort
* @deprecated since 1.7.0, will be removed in 1.12.0; This method does only best-effort

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you know any alternative way to invalidate all entries in the cache?

Depends on the goal.
if you invalidate just to reduce memory usage (rare), this method is fine
if you invalidate because something changed and all values needs to be recomputed (including those in flight), then there is no ready to use solution.

In trino we have additional layer on top of Guava caches that solves this problem. Here we use caffeine, but last time i checked, it had the same invalidation race problem and the author's feedback was that it was fundamentally not solvable.
thus, if full invalidation is needed, you can either copy code from trino-cache trino module (or use the code i already extracted https://github.com/findepi/evictable-cache)

Here i'd hope that invalidation isn't really needed for anything and we can remove the API.

@gaborkaszab
gaborkaszabforce-pushed the main_change_deprecations_core branch from 82673e6 to 5bcdeabCompareDecember 10, 2025 14:15
@pvary

Copy link
Copy Markdown
Contributor

Let's wait a bit more if somebody has any more comments

* @deprecated Use {@link AuthProperties#AUTH_TYPE}={@link AuthProperties#AUTH_TYPE_SIGV4}
* instead. Will be removed in 1.12.0.
*/
@Deprecated private static final String SIGV4_ENABLED_LEGACY = "rest.sigv4-enabled";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is actually a fallback mechanism so that older clients still work. When this property is removed, we want to make sure to switch the warn to an error msg but still leave the fallback mechanism in-place

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Taking a second look, I just realize this is a private member, so technically this is already deprecated. I'll revert the changes here. because there isn't much we can do atm.

@gaborkaszab
gaborkaszabforce-pushed the main_change_deprecations_core branch from 5bcdeab to fc02d3aCompareDecember 12, 2025 14:47
@pvary
pvary merged commit 831b4ea into apache:mainDec 15, 2025
44 checks passed
@pvary

Copy link
Copy Markdown
Contributor

Merged to main.
Thanks @gaborkaszab for the PR and @nastra, @findepi, @ggershinsky for the review

talatuyarer pushed a commit to talatuyarer/iceberg that referenced this pull request Apr 1, 2026
dramaticlly added a commit to dramaticlly/iceberg that referenced this pull request Aug 26, 2026
…data to 1.13.0
The removal of the ability to write position deletes with row data (PDWR) was
scheduled for 1.12.0 (see apache#17706). The 1.12.0 release candidate is being cut
now and the removal touches public API across Core, Data and Spark 3.5/4.0/4.1,
so it should not be rushed through review.
This moves the removal target of those deprecations from 1.12.0 to 1.13.0. The
deprecations themselves are unchanged: everything is still deprecated as of
1.11.0 and callers should still migrate off the row-carrying overloads. Only
the stated removal release moves out by one cycle. This mirrors apache#14392, which
retargeted a batch of removal versions in the other direction.
Markers moved to 1.13.0:
- PositionDelete.set(CharSequence, long, R) and row()
- PositionDeleteWriter constructor (appender type narrowing)
- RewriteTablePathUtil.PositionDeleteReaderWriter.writer(..., Schema)
- GenericFileWriterFactory positionDeleteRowSchema constructors, the builder
setter, and the nine configureDataWrite/configureEqualityDelete/
configurePositionDelete methods superseded by FormatModelRegistry, whose
removal was bundled into the same change
- SparkFileWriterFactory positionDeleteRowSchema/positionDeleteSparkType
builder setters, the row-schema constructor, and the runtime warning message
- SparkPositionDeltaWrite.Context.deleteSparkType()
Additionally, PositionDeltaWriter.delete(CharSequence, long, T, PartitionSpec,
StructLike) is now deprecated for removal in 1.13.0. It was previously not
deprecated at all, so without this its deprecation cycle would only start in
1.13.0 and the row parameter could not be dropped until 1.14.0. Deprecating it
here keeps that narrowing on schedule for 1.13.0 alongside the rest. The
annotation is additive: the overload stays abstract, so implementations are
unaffected, while callers can migrate to the four-argument overload today. All
three SparkPositionDeltaWrite call sites already use it; the only override is
BasePositionDeltaWriter, which already calls the deprecated
PositionDelete.set(CharSequence, long, R).
Flink is not deferred. Its only PDWR surface was FlinkAppenderFactory, whose
entire position delete path is built on row data. That class was deprecated as
of 1.11.0 in favor of FlinkFileWriterFactory for removal in 1.12.0, and nothing
in the repo references it outside its own test, so it is removed here on
schedule from all three Flink versions rather than carried into 1.13.0. That
also drops org.apache.iceberg.io.TestAppenderFactory from iceberg-data: the
three TestFlinkAppenderFactory classes were its only subclasses, so with those
gone the abstract base exercises nothing. iceberg-flink is not a revapi project
and TestAppenderFactory is a test class, so neither removal needs a revapi
baseline entry.
The REST scan-planning markers in BaseScanTaskResponse, PlanTableScanRequest
and PlanTableScanResponse are left alone; they are handled separately in apache#17638.
huaxingao pushed a commit that referenced this pull request Sep 1, 2026
…log message (#17812)
* Core: Remove ThreadPools.WORKER_THREAD_POOL_SIZE_PROP and correct manifest spec fallback removal version
Resolves the two remaining deprecation markers in iceberg-core that
promised action "in 1.12.0" but were missed by the removal PRs.
Remove ThreadPools.WORKER_THREAD_POOL_SIZE_PROP. The field was
deprecated in #5659 in favor of SystemConfigs.WORKER_THREAD_POOL_SIZE
and retargeted from 2.0.0 to 1.12.0 by #14392. It had no callers
anywhere in the repository outside its own declaration and no
references in the docs, and it was the only survivor of #14392's
13-file batch after #17700 removed the rest. The corresponding
java.field.removed break is recorded in .palantir/revapi.yml under the
1.11.0 baseline.
Correct the ManifestReader warning for the partition-spec-from-file-
metadata fallback to say 2.0.0 rather than 1.12.0. The fallback cannot
be honored in 1.12.0 because its only production caller is
BaseSnapshot:276, which passes a null spec map while serving
Snapshot.addedDeleteFiles(FileIO) and removedDeleteFiles(FileIO).
Those accessors are themselves already marked "@deprecated will be
removed in 2.0.0; use SnapshotChanges#builderFor(Table) instead", and
because they receive only a FileIO they structurally cannot supply a
spec map. The fallback must therefore outlive 1.12.0, and 2.0.0 aligns
its removal with that of its last caller.
* reword revapi change
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@gaborkaszab@pvary@findepi@nastra@ggershinsky