Skip to content

Comments

pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config#25429

Merged
IoannisRP merged 1 commit intoredpanda-data:devfrom
IoannisRP:CORE-9525/always_normalize
Mar 24, 2025
Merged

pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config#25429
IoannisRP merged 1 commit intoredpanda-data:devfrom
IoannisRP:CORE-9525/always_normalize

Conversation

@IoannisRP
Copy link
Contributor

@IoannisRP IoannisRP commented Mar 19, 2025

Implements: CORE-9526

The new functionality allows to blanket-enable schema normalization in every SR operation.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v25.1.x
  • v24.3.x
  • v24.2.x
  • v24.1.x

Release Notes

  • none

@IoannisRP IoannisRP changed the title pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config [draft] pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config Mar 19, 2025
@IoannisRP IoannisRP force-pushed the CORE-9525/always_normalize branch from cd7148c to 91b0184 Compare March 20, 2025 13:53
@IoannisRP IoannisRP changed the title [draft] pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config pandaproxy/sr: Re-purpose schema_registry_normalize_on_startup config Mar 20, 2025
@IoannisRP IoannisRP marked this pull request as ready for review March 20, 2025 13:56
@IoannisRP IoannisRP requested a review from a team as a code owner March 20, 2025 13:56
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Mar 20, 2025

CI test results

test results on build#63437
test_id test_kind job_url test_status passed
rptest.tests.datalake.batching_test.DatalakeBatchingTest.test_batching.cloud_storage_type=CloudStorageType.S3.query_engine=QueryEngineType.SPARK.catalog_type=CatalogType.REST_JDBC.expect_large_files=True ducktape https://buildkite.com/redpanda/redpanda/builds/63437#0195b435-5dcd-44b3-8512-3691cfb40706 FLAKY 1/2
rptest.tests.datalake.compaction_test.CompactionTest.test_compaction.cloud_storage_type=CloudStorageType.S3.catalog_type=CatalogType.REST_JDBC ducktape https://buildkite.com/redpanda/redpanda/builds/63437#0195b435-5dd0-4672-b43b-fe9bcd49db2a FLAKY 1/2
rptest.tests.datalake.recovery_mode_test.DatalakeRecoveryModeTest.test_recovery_mode.cloud_storage_type=CloudStorageType.S3.catalog_type=CatalogType.NESSIE ducktape https://buildkite.com/redpanda/redpanda/builds/63437#0195b432-b329-48d1-aac0-a01ba06889a7 FLAKY 1/2
test results on build#63493
test_id test_kind job_url test_status passed
rptest.tests.data_migrations_api_test.DataMigrationsApiTest.test_migrated_topic_data_integrity.transfer_leadership=False.params=.cancellation.dir.in.stage.preparing.use_alias.True ducktape https://buildkite.com/redpanda/redpanda/builds/63493#0195b875-67c5-44ed-b517-82b4c7ba75fc FLAKY 1/2
rptest.tests.data_migrations_api_test.DataMigrationsApiTest.test_migrated_topic_data_integrity.transfer_leadership=True.params=.cancellation.dir.in.stage.preparing.use_alias.True ducktape https://buildkite.com/redpanda/redpanda/builds/63493#0195b875-67c7-422a-b366-9d30e6b37e50 FLAKY 1/3
rptest.tests.datalake.datalake_upgrade_test.DatalakeUpgradeTest.test_upload_through_upgrade.cloud_storage_type=CloudStorageType.S3.query_engine=QueryEngineType.SPARK ducktape https://buildkite.com/redpanda/redpanda/builds/63493#0195b878-5cb5-4a11-adb6-b82f2f4120e3 FLAKY 1/2
rptest.tests.scaling_up_test.ScalingUpTest.test_scaling_up_with_recovered_topic ducktape https://buildkite.com/redpanda/redpanda/builds/63493#0195b878-5cb5-4a11-adb6-b82f2f4120e3 FLAKY 1/2

Copy link
Contributor

@kbatuigas kbatuigas left a comment

Choose a reason for hiding this comment

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

Just a suggestion for clarity in docs 👍

The new functionality allows to blanket-enable schema normalization
in every SR operation.
@IoannisRP IoannisRP force-pushed the CORE-9525/always_normalize branch from 91b0184 to a17d273 Compare March 21, 2025 09:48
@IoannisRP IoannisRP requested a review from kbatuigas March 21, 2025 09:55
@IoannisRP
Copy link
Contributor Author

Changes in force-push:

  • Updated schema_registry_always_normalize description

"schema_registry_always_normalize",
"Always normalize schemas. If set, this overrides the "
"normalize parameter in API requests.",
{.needs_restart = needs_restart::yes,
Copy link
Member

Choose a reason for hiding this comment

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

I see no reason not to make this configurable at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no intrinsic reason as to why we can't flip it, apart from ease of change.

We call the sharded_store constructor in quite a few places, and all of them would need either a binding or a mock_binding for tests. It would add lot's of noise to the pr. I would prefer to make up a follow up for that, with just that change.

@IoannisRP IoannisRP requested a review from BenPope March 24, 2025 13:58
@IoannisRP IoannisRP merged commit 2c47a47 into redpanda-data:dev Mar 24, 2025
20 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v25.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v24.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v24.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-25429-v24.3.x-308 remotes/upstream/v24.3.x
git cherry-pick -x a17d27353d

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v24.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-25429-v24.2.x-324 remotes/upstream/v24.2.x
git cherry-pick -x a17d27353d

Workflow run logs.

"normalize parameter in API requests.",
{.needs_restart = needs_restart::yes,
.visibility = visibility::user,
.aliases = {"schema_registry_normalize_on_startup"}},
Copy link
Member

@dotnwat dotnwat Mar 24, 2025

Choose a reason for hiding this comment

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

@IoannisRP @BenPope is the expectation that schema_registry_always_normalize will take on the previous value of schema_registry_normalize_on_startup after upgrade because you've set .aliases? If so, have you checked, because I don't think behavior exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dotnwat previous behavior was kinda trampled by #25217 and reintroduced here.

Copy link
Member

Choose a reason for hiding this comment

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

@IoannisRP i'm having trouble understanding how that PR relates to my question. can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, misunderstood the question. Yeah, just checked. It does respect the old value after an update.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, misunderstood the question. Yeah, just checked. It does respect the old value after an update.

@IoannisRP so i still have the same question: what is the expectation? is the expectation that if schema_registry_normalize_on_startup were set to true (a non-default value) prior to upgrading to this code, that the new configuration would also be true? if so, i don't think that it works that way. did you test that scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the expectation that if schema_registry_normalize_on_startup were set to true (a non-default value) prior to upgrading to this code, that the new configuration would also be true?

yes, the expectation is that the new config will respect the potentially non-default value of the old alias.

if so, i don't think that it works that way. did you test that scenario?

it did test this scenario, yes. I run an older rp without my changes and set schema_registry_normalize_on_startup to true. Then I run a rp with my changes and it reports schema_registry_always_normalize as true

You can also check any other PR where an alias replaces an existing config. They all just rename the variable and keep the old name as a part in .aliases.

Copy link
Member

Choose a reason for hiding this comment

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

@IoannisRP huh, indeed. i tried it again and it does seem to be working this way. im not sure how i messed up the test before. sorry for the noise!

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.

5 participants