Add annotation @BehaviorChangeSince for behavior-change flags by eric-maynard · Pull Request #1170 · apache/polaris

eric-maynard · 2025-03-13T16:49:37Z

#1124 introduces "behavior change" flags, which are intended to guard contentious or risky behavior changes and to be removed after a short time. This PR codifies that intention by adding a new spotless rule checkStaleBehaviorChangeFlags that will fail during format when a behavior change flag is detected to have been around for more than 2 minor release or for more than 1 major release.

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfigurationSince.java

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java

dimas-b · 2025-03-13T21:59:12Z

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfigurationSince.java

+import java.lang.annotation.Target;
+
+/**
+ * An annotation that should be applied to all BehaviorChangeConfiguration instances. The value here


Should we add validation for that to the spotless check too?

Definitely. I looked into this a bit, but it seems like the "best" way would be via reflection (enumerating all instances of the BehaviorChangeConfiguration type) which feels hacky. At the very least, let me add a check so that no configs are created outside of the correct file.

That can be done in a compile-time annotation processor (I think it has access to all elements, even if not annotated).

Awesome, I'll look into that and update the PR. For now, I added a lint check to try and make sure the flags are contained within the one file.

After some research, I have one doubt about the above approach. If we want to scan for BehaviorChangeConfiguration instances without the annotation, is an annotation processor really the right choice? I am wondering if this, too, needs to be done in the linter.

Cross references are inevitable. For example, immutables generate code from annotations that is referenced by some other hand-written classes. @Overrides annotations relate to base classes, which do not have the annotation itself. I think it is quite normal for annotation processors to "look around" in the codebase.

By the way, the immutables lib can be a source of inspiration here, I guess 😉

I tried adding a processor BehaviorChangeProcessor and registering it, but I don't think it actually runs based on my testing. If you're able to take a look and you see a problem, please do let me know

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfigurationSince.java

dimas-b · 2025-03-14T12:54:17Z

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java

    super(key, description, defaultValue, catalogConfig);
  }

+  @BehaviorChangeConfigurationSince("1.0.0")


We need to coordinate setting this value with the new 0.10.0 release proposal: https://lists.apache.org/thread/8kc7xq3lp6pcc2w3jon3fv8qfxj3v9lk

We need to fix the versions.txt to point to the actual version of the code, I think. These values should be set to whatever is there. Currently, that's 1.0.0

dimas-b · 2025-03-14T13:29:52Z

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfigurationSince.java

+ */
+@Target(ElementType.FIELD)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface BehaviorChangeConfigurationSince {


Side note: it would be great to consider this annotation in tools/config-docs/generator

Do you mean to move it there?

No, I mean auto-generated config docs should ideally consider this annotation

I see, I've looked through the generator package but I'm not sure I fully understand what it would mean for the docs to consider the annotation. I guess the idea would be to have the docs about each BehaviorChangeConfiguration describe what version the configuration was created in, etc.? In fact, I don't know if we even want to document the BehaviorChangeConfigurations

I believe, in an open source project all config that a user may want to change should be documented... but we can leave BehaviorChangeConfigurations for later, of course.

dimas-b · 2025-03-17T21:29:08Z

build-logic/src/main/kotlin/polaris-java.gradle.kts

+
+  override fun apply(input: String): String {
+    val versionRegex =
+      """@BehaviorChangeScope\s*\(\s*since\s*=\s*"(\d+\.\d+\.\d+)"(?:\s*,\s*expires\s*=\s*"(\d+\.\d+\.\d+)")?\s*\)"""


Will it work if the annotation is split across multiple lines?

It more or less should work because of all the \s* -- but agreed that a lint check is relatively brittle here. We can always add another check later though

build-logic/src/main/kotlin/polaris-java.gradle.kts

dimas-b · 2025-03-17T21:33:04Z

build-logic/src/main/kotlin/polaris-java.gradle.kts

+    val (majorA, minorA, patchA) = annotatedVersion.split(".").map { it.toInt() }
+    if (expiresVersion == null) {
+      val (majorB, minorB, _) = polarisVersion.split(".").map { it.toInt() }
+      return (majorB > majorA || (majorB == majorA && minorB > minorA + 1))


Is it possible to have only on place for comparing versions, but derive the "expires" version from the "added" version automatically?

I updated the PR to try this, but the logic might be less intuitive at first glance. Let me know what you think!

build-logic/src/main/kotlin/polaris-java.gradle.kts

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfiguration.java

dimas-b · 2025-03-17T21:38:47Z

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeProcessor.java

+                        Diagnostic.Kind.NOTE,
+                        "Field "
+                            + field.getSimpleName()
+                            + " is correctly annotated with @BehaviorChange",


Is it worth logging?

I think it's not, but I noticed the processor isn't working so I added this to debug -- see my comment here. If you have any tips on fixing this, that would be awesome. I think the annotation processor is a good suggestion

dimas-b · 2025-03-17T21:41:42Z

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeProcessor.java

+public class BehaviorChangeProcessor extends AbstractProcessor {
+
+  @Override
+  public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {


Would it be possible to move version/expiry checks here? It think it would be nicer from the overall design POV.

Build scripts could probably inject "current version" into system properties. WDYT?

I really prefer to do things like this in a linter. We can always do it in both, too.

having both looks awkward to me... I'd prefer to move the logic and keep only one of them (up to you which one).

dimas-b · 2025-03-31T13:59:10Z

@eric-maynard : how about converting built-time checks to unit tests? I believe tests can scan pre-compiled classes for annotations and verity that all required constants are annotated. Also we can probably inject the current version number from gradle and validate expiry... as for me it looks simpler and easier to debug that build-time checks... WDYT?

eric-maynard · 2025-04-01T16:52:20Z

Good idea @dimas-b, I will try to do just that. Putting the PR in draft for now

github-actions · 2025-05-02T02:08:12Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

eric-maynard added 5 commits March 12, 2025 12:10

wip

f744263

add gradle rule

aa23522

passing

674467f

fix logic

5a9ea4a

some improvements

2c95c2b

eric-maynard requested review from adutra, ashvina, collado-mike, dennishuo, dimas-b, jackye1995, jbonofre and vvcephei as code owners March 13, 2025 16:49

github-project-automation bot added this to Basic Kanban Board Mar 13, 2025

eric-maynard requested review from MonkeyCanCode, RussellSpitzer, ebyhr, flyrain, snazy and takidau as code owners March 13, 2025 16:49

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 13, 2025

dimas-b reviewed Mar 13, 2025

View reviewed changes

eric-maynard added 2 commits March 13, 2025 13:46

changes per review

30a2c2d

autolint

7cc7e7b

dimas-b reviewed Mar 13, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/config/BehaviorChangeConfigurationSince.java Outdated Show resolved Hide resolved

add another lint

10eef5b

dimas-b reviewed Mar 14, 2025

View reviewed changes

flashier comment

55b33e8

eric-maynard added 4 commits March 14, 2025 10:20

autolint

9f376b8

changes per review

c4299e3

Add annotation processor

2a059d7

uncomment annotation

49a955d

dimas-b reviewed Mar 17, 2025

View reviewed changes

changes

8c163b1

eric-maynard mentioned this pull request Mar 19, 2025

Fix behavior change config VALIDATE_VIEW_LOCATION_OVERLAP #1201

Merged

eric-maynard requested a review from dimas-b March 26, 2025 17:53

eric-maynard marked this pull request as draft April 1, 2025 16:52

github-actions bot added the stale label May 2, 2025

eric-maynard closed this May 21, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board May 21, 2025

Conversation

eric-maynard commented Mar 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-maynard Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Mar 31, 2025

Uh oh!

eric-maynard commented Apr 1, 2025

Uh oh!

github-actions bot commented May 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eric-maynard Mar 19, 2025 •

edited

Loading