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

Clean name that cluster #498

Merged
merged 15 commits into from
Aug 1, 2023
Merged

Clean name that cluster #498

merged 15 commits into from
Aug 1, 2023

Conversation

SamBarker
Copy link
Member

@SamBarker SamBarker commented Jul 30, 2023 •
edited
Loading

Type of change

  • Enhancement / new feature

Description

Includes the a name in the Virtual Cluster configuration.

Additional Context

We need a programatic way of identifying which cluster is being used. This allows us to:

  • share state at a cluster level.
  • Improve logging so its easier to identify which clusters are being used

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Review performance test results. Ensure that any degradations to performance numbers are understood and justified.
  • Make sure all Sonar-Lint warnings are addressed or are justifiably ignored.
  • Update documentation
  • Reference relevant issue(s) and close them after merging
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

@robobario
Copy link
Contributor

Should we validate that the VCs are uniquely named somewhere?

@SamBarker
Copy link
Member Author

Should we validate that the VCs are uniquely named somewhere?

Yeah for operator sanity/error handling. Currently the last (as seen determined by jackson) cluster with the name will win.

SamBarker added 3 commits July 31, 2023 12:55
@SamBarker
Optionally name clusters.
0dbde6e 
Currently not sure about making the name optional, but it avoids this being a breaking change.
@SamBarker
Wire virtual cluster name into the model hierarchy
cd03716
@SamBarker
Refactoring. Code cleanups.
71b29ba
@SamBarker SamBarker force-pushed the clean_name_that_cluster branch 2 times, most recently from 065b1bd to aeebe19 Compare July 31, 2023 04:29
@k-wall
Copy link
Contributor

k-wall commented Jul 31, 2023

Thanks for doing this @SamBarker. This is also going to pave the way to having metrics distinguished virtual cluster name too.

k-wall
k-wall approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

Apart from my question above about whether retaining the virtualClusters as a map is a valuable, lgtm.

@SamBarker SamBarker force-pushed the clean_name_that_cluster branch from 8901a65 to e57234a Compare July 31, 2023 22:30
SamBarker added 10 commits August 1, 2023 11:46
@SamBarker
Update CHANGELOG.md
38b2165 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Shuffle config model translation from Proxy to Model Object
73dd374 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Validate cluster names are unique
73156c4 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Enable JsonParser.Feature.STRICT_DUPLICATE_DETECTION so that we det…
84a1570 
…ect duplicate virtual cluster names.

Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Switch to more specific exception type to keep Sonar from whinning.
14b85fb 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Drop support for clusterName nodes as we don't have any need for it.
c821f64 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Switch to using the validation hook to acquire cluster configuration.
45d6d35 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Make Sonar shutup in the port deconflictor.
94faa17 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Fix sonar warnings in ExpositionIT
010cf5f 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
Intellij and maven formatter disagree
8bca278 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker SamBarker force-pushed the clean_name_that_cluster branch from e57234a to 8bca278 Compare July 31, 2023 23:46
robobario
robobario reviewed Aug 1, 2023
View reviewed changes
kroxylicious/src/main/java/io/kroxylicious/proxy/config/Configuration.java Outdated
@@ -26,4 +26,10 @@ public List<MicrometerDefinition> getMicrometer() {
public boolean isUseIoUring() {
return useIoUring();
}

public List<io.kroxylicious.proxy.model.VirtualCluster> validClusters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still be validClusters when it doesn't filter anything? It sounds like it returns a subset of clusters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a good question 😁

I added it so I had somewhere post config parsing to validate the cluster names were unique, and I still feel that it represents a valid and sensible hook to have. However I haven't figured out how to include the port de-conflictor here. Maybe I should just back it out entirely as we are moving to blowing up on invalid config.

robobario
robobario approved these changes Aug 1, 2023
View reviewed changes
Copy link
Contributor

@robobario robobario left a comment

Choose a reason for hiding this comment

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

LGTM

@SamBarker
Remove cluster name from example config.
e5d7760 
Signed-off-by: Sam Barker <[email protected]>
@SamBarker
validClusters -> virtualClusterModel.
249cd34 
As we don't do any validation or filtering however the model conversion hook is still valuable.

Signed-off-by: Sam Barker <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@SamBarker SamBarker merged commit 04d127d into kroxylicious:main Aug 1, 2023
@SamBarker SamBarker deleted the clean_name_that_cluster branch August 1, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k-wall k-wall k-wall approved these changes

@robobario robobario robobario approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Kroxylicious
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SamBarker @robobario @k-wall