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 from a945657 to 9d91394 Compare January 21, 2025 15:30
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 9d91394 to 3dc8d19 Compare January 23, 2025 07:05
@Ivan-Montes Ivan-Montes force-pushed the feature/set-eureka-instanceinfo-to-endpoint-attr branch 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

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.

Set Eureka InstanceInfo to endpoint attribute
3 participants