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

Set Eureka InstanceInfo to endpoint attribute #6069

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Ivan-Montes
Copy link

Related: #6056

Motivation:

Users might want to use the metadata from the Eureka InstanceInfo but currently, there's no way to retrieve it.

Modifications:

  • Add a helper class to hide the implementation detail.
  • Set the InstanceInfo to the Endpoint as an attribute.

Result:

@CLAassistant
Copy link

CLAassistant commented Jan 13, 2025

CLA assistant check
All committers have signed the CLA.

@Ivan-Montes Ivan-Montes force-pushed the feature/set-eureka-instanceinfo-to-endpoint-attr branch 2 times, most recently from d205a96 to 258fd3f Compare January 15, 2025 09:58
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Would you add a test that retrieve an InstanceInfo from EurekaEndpointGroup?
I think we can use EurekaEndpointGroupTest class.

@Ivan-Montes Ivan-Montes force-pushed the feature/set-eureka-instanceinfo-to-endpoint-attr branch 3 times, most recently from 3dc8d19 to 9a88c9d Compare January 24, 2025 12:18
@Ivan-Montes
Copy link
Author

Would you add a test that retrieve an InstanceInfo from EurekaEndpointGroup? I think we can use EurekaEndpointGroupTest class.

I’ve made the requested changes. Please have a look when you get a chance! Thanks @minwoox

@minwoox minwoox added this to the 1.32.0 milestone Feb 4, 2025
Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Looks good to me, @Ivan-Montes
Thanks! 😄

Related: line#6056

Motivation:

Users might want to use the metadata from the Eureka `InstanceInfo` but currently, there's no way to retrieve it.

Modifications:

- Add a helper class to hide the implementation detail.
- Set the `InstanceInfo` to the `Endpoint` as an attribute.

Result:

- Closes line#6056
- Now users can retrieve it.
Related: line#6056

Motivation:

Users might want to use the metadata from the Eureka `InstanceInfo` but currently, there's no way to retrieve it.

Modifications:

- Remove helper class because users cannot access this class to retrieve `InstanceInfo`.
- Move `com.linecorp.armeria.internal.common.eureka.InstanceInfo` to `com.linecorp.armeria.common.eureka.InstanceInfo`.
- Move the methods to the `InstanceInfo` class.
- Add a test to verify that users can retrieve the `InstanceInfo`.

Result:

- Closes line#6056
- Now users can retrieve it.
Motivation:

Since it is ensured that the public API does not expose the shaded classes, after moving `com.linecorp.armeria.internal.common.eureka.InstanceInfo` to `com.linecorp.armeria.common.eureka.InstanceInfo`, the same exception occurs with the following jobs:
- build-ubicloud-standard-16-jdk-*
- build-macos-latest-jdk-21
- build-windows-latest-jdk-21

```
Execution failed for task ':javadoc:checkJavadoc'.
> java.lang.Exception: Disallowed class(es) in the public API:
  - InstanceInfo -> com.linecorp.armeria.internal.common.eureka.DataCenterInfo
  - InstanceInfo -> com.linecorp.armeria.internal.common.eureka.LeaseInfo
```

Modifications:

- Move `com.linecorp.armeria.internal.common.eureka.DataCenterInfo` to `com.linecorp.armeria.common.eureka.DataCenterInfo`.
- Move `com.linecorp.armeria.internal.common.eureka.LeaseInfo` to `com.linecorp.armeria.common.eureka.LeaseInfo`.
- Make public `com.linecorp.armeria.internal.common.eureka.DataCenterInfoSerializer`

Result:

DataCenterInfo and LeaseInfo classes are now allowed
@Ivan-Montes Ivan-Montes force-pushed the feature/set-eureka-instanceinfo-to-endpoint-attr branch from 1575885 to ee0f298 Compare February 22, 2025 10:29
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.

Set Eureka InstanceInfo to endpoint attribute
4 participants