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.

Sign up for GitHub

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

Jump to bottom

Edit topic with advanced configs #1414

Merged
merged 7 commits into from
Jun 28, 2023
Merged

Edit topic with advanced configs #1414

merged 7 commits into from
Jun 28, 2023

Conversation

muralibasani
Copy link
Contributor

@muralibasani muralibasani commented Jun 27, 2023 •
edited
Loading

About this change - What it does

  • For edit topic requests, updates advanced config if present
  • In topic overview, retrieve and display advanced config
  • for edit topic requests, display if any advanced config exists

Resolves: #1392
Why this way
Edit topic requests now can handle other configs

muralibasani added 2 commits June 27, 2023 15:58
Edit topic with advanced configs
d8a8ee0 
Signed-off-by: muralibasani <[email protected]>
Add tests
958d120 
Signed-off-by: muralibasani <[email protected]>
@muralibasani muralibasani marked this pull request as ready for review June 27, 2023 15:31
@muralibasani muralibasani requested a review from a team as a code owner June 27, 2023 15:31
@muralibasani
Copy link
Contributor Author

image

@muralibasani
Copy link
Contributor Author

image

aindriu-aiven
aindriu-aiven approved these changes Jun 27, 2023
View reviewed changes
Copy link
Contributor

@aindriu-aiven aindriu-aiven left a comment

Choose a reason for hiding this comment

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

Changes look good

core/src/main/java/io/aiven/klaw/service/TopicOverviewService.java Outdated
topicInfo.setAdvancedTopicConfiguration(
topicConfigurationRequest.getAdvancedTopicConfiguration());
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth creating a KlawException that is caught in the controller advice that returns a bad request with a message per processable advanced config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good one. updated, but now suppressing that error. what do you think.

@aindriu-aiven
Copy link
Contributor

aindriu-aiven commented Jun 27, 2023 •
edited
Loading

Another note is that the topic Replication factor is not auto filled in on trying to edit.
And the advanced config didnt fill for me either is that expected?
image

@muralibasani
Copy link
Contributor Author

Another note is that the topic Replication factor is not auto filled in on trying to edit. And the advanced config didnt fill for me either is that expected? image

Yea, it is expected, as this advanced config is never saved before. We were directly applying changes on cluster, but saving in metadata

Update rep factor
c82d418 
Signed-off-by: muralibasani <[email protected]>
@muralibasani muralibasani requested a review from a team as a code owner June 27, 2023 16:42
@muralibasani
Copy link
Contributor Author

Another note is that the topic Replication factor is not auto filled in on trying to edit. And the advanced config didnt fill for me either is that expected? image

Fixed, should be ok now.

@muralibasani muralibasani merged commit dd1bd11 into main Jun 28, 2023
@muralibasani muralibasani deleted the fix-edit branch June 28, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aindriu-aiven aindriu-aiven aindriu-aiven approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update topic properties via topic edit button
2 participants
@muralibasani @aindriu-aiven