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

Consume ServiceInfo data building ServiceInfoData structure #299

Open
Luni-4 opened this issue Feb 4, 2025 · 2 comments
Open

Consume ServiceInfo data building ServiceInfoData structure #299

Luni-4 opened this issue Feb 4, 2025 · 2 comments

Comments

@Luni-4
Copy link

Luni-4 commented Feb 4, 2025

Currently, I'm facing a specific workflow which needs to consume ServiceInfo. In my opinion, it would be helpful to add a ServiceInfoData structure containing all information that can be retrieved through get_* functions.

Something like:

impl ServiceInfo {
  pub fn service_info_data(self) -> ServiceInfoData {
      ServiceInfoData {
             fullname: self.fullname,
             addresses: self.addresses,
             port: self.port,
             properties: self.properties.properties_data(),
             ...
      }
  }
}

The example above is not exhaustive, therefore some public fields are missing. This approach would reduce the amount of allocations to be performed when ServiceInfo must not be saved.

@keepsimple1
Copy link
Owner

keepsimple1 commented Feb 5, 2025

You have a good point. Do you intend to use ServiceInfoData only on the client (browser) side?

I think the problem is that currently the same ServiceInfo struct is used in both the server and the client, i.e. in publishing a service (register) and in resolving a service instance (ServiceEvent::ServiceResolved)

We probably should have defined a different struct for the client side in resolving a service, that contains the attributes as in ServiceInfoData you mentioned. Assuming you intend to use ServiceInfoData only on the client side, I think there are a couple of options going forward:

  1. Like you suggested, add a new method in ServiceInfo to return a new struct. The only thing is that I felt the name ServiceInfoData could cause confusions, as data is quite generic and similar to info.

  2. Replace the struct in ServiceResolved(ServiceInfo) with a new struct that is smaller than ServiceInfo and it's clearly only used on the client side. The downside is that it will be a breaking change, even if we provide all same getter functions of ServiceInfo.

In both options, how about naming the new struct ResolvedService ? It clearly indicates the purpose of the struct, and will only used by the client side.

What do you think?

@Luni-4
Copy link
Author

Luni-4 commented Feb 5, 2025

You have a good point. Do you intend to use ServiceInfoData only on the client (browser) side?

Yep, exactly, only on the client side.

I think the problem is that currently the same ServiceInfo struct is used in both the server and the client, i.e. in publishing a service (register) and in resolving a service instance (ServiceEvent::ServiceResolved)

We probably should have defined a different struct for the client side in resolving a service, that contains the attributes as in ServiceInfoData you mentioned. Assuming you intend to use ServiceInfoData only on the client side, I think there are a couple of options going forward:

1. Like you suggested, add a new method in `ServiceInfo` to return a new struct.  The only thing is that I felt the name `ServiceInfoData` could cause confusions, as `data` is quite generic and similar to `info`.

2. Replace the struct in `ServiceResolved(ServiceInfo)` with a new struct that is smaller than `ServiceInfo` and it's clearly only used on the client side.  The downside is that it will be a breaking change, even if we provide all same `getter` functions  of `ServiceInfo`.

In both options, how about naming the new struct ResolvedService ? It clearly indicates the purpose of the struct, and will only used by the client side.

What do you think?

I agree with differentiating server and client side because they deal with separate things. I really like the first option because it's simpler and ResolvedService is a good name in my opinion. However, I cannot figure out the breaking change issue with the second option though, perhaps are there some problems, that I am not aware of, if we publish the 0.14.0 version?

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

No branches or pull requests

2 participants