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

DGS-7208 DGS-7474 Add alias/normalize configs #2664

Merged
merged 6 commits into from Jun 8, 2023

Conversation

rayokota
Copy link
Member

@rayokota rayokota commented Jun 7, 2023

An alias allows you to look up a subject using an alternative name.

The normalize config allows you to ensure normalization is always used for registration/lookup calls.

Also add QualifiedReferenceSubjectNameStrategy which is an alternative implementation of ReferenceSubjectNameStrategy.

An alias allows you to look up a subject using an alternative name.

The normalize config allows you to ensure normalization is always
used for registration/lookup calls.
@rayokota rayokota requested a review from a team as a code owner June 7, 2023 19:42
Copy link
Member

@akhileshm1 akhileshm1 left a comment

Choose a reason for hiding this comment

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

thanks @rayokota. LGTM. few questions for my own understanding.

Comment on lines +38 to +40
private final KafkaSchemaRegistry schemaRegistry;

public AliasFilter(KafkaSchemaRegistry schemaRegistry) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can these be SchemaRegistry?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the method getConfig(String subject) is not on SchemaRegistry.

@PreMatching
@Priority(Priorities.ENTITY_CODER + 100) // ensure runs after ContextFilter
public class AliasFilter implements ContainerRequestFilter {
private static final Logger log = LoggerFactory.getLogger(AliasFilter.class);
Copy link
Member

Choose a reason for hiding this comment

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

nit: log doesn't seem to be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove

String modifyUriPath(String path) {
boolean subjectPathFound = false;
StringBuilder modifiedPath = new StringBuilder();
boolean isFirst = true;
Copy link
Member

Choose a reason for hiding this comment

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

this is being set to false below but I'm not sure if I understand the significance of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove

config,
restApp.restClient.updateConfig(config, null));

Schema schema = restApp.restClient.getVersion("testSubject", 1);
Copy link
Member

Choose a reason for hiding this comment

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

should this be badSubject?

Copy link
Member Author

Choose a reason for hiding this comment

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

This test tests that aliases set at the global level (with no subject specified) don't do anything. The test is called testGlobalAliasNotUsed. In the future we may want to use these for something, but for now they are not used.

}

@Test
public void testRoot() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Exception is never thrown

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll remove

rayokota and others added 2 commits June 7, 2023 15:22
…ers/AliasFilter.java

Co-authored-by: Akhilesh Manjunath <am5156@cs.nyu.edu>
…ers/AliasFilterTest.java

Co-authored-by: Akhilesh Manjunath <am5156@cs.nyu.edu>
@rayokota
Copy link
Member Author

rayokota commented Jun 7, 2023

thanks @rayokota. LGTM. few questions for my own understanding.

Thanks for the detailed review and feedback @akhileshm1 !

@rayokota rayokota merged commit 09fe82b into confluentinc:7.4.x Jun 8, 2023
3 checks passed
@rayokota rayokota deleted the DGS-7208 branch June 8, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants