Introduce type parameters where applicable for xDS classes. #5410

minwoox · 2024-01-26T01:01:49Z

Motivation:
Following the discussion in this comment, incorporating type parameters in xDS classes offers advantages such as improved type safety and enhanced readability. Modifications:

Introduce type parameters where applicable.
Split AbstractResourceNode into AbstractResourceNode and AbstractResourceNodeWithPrimer to accommodate the absence of a primer in the listener.
Rename AbstractResourceHolder to ResourceHolderWithPrimer for consistency due to the absence of a primer in the listener.
Conduct clean-up in some APIs. Result:
Enhanced type safety, improved readability, and a more structured representation in xDS classes.

Motivation: Following the discussion in this [comment](line#5336 (comment)), incorporating type parameters in xDS classes offers advantages such as improved type safety and enhanced readability. Modifications: - Introduce type parameters where applicable. - Split `AbstractResourceNode` into `AbstractResourceNode` and `AbstractResourceNodeWithPrimer` to accommodate the absence of a primer in the listener. - Rename `AbstractResourceHolder` to `ResourceHolderWithPrimer` for consistency due to the absence of a primer in the listener. - Conduct clean-up in some APIs. Result: - Enhanced type safety, improved readability, and a more structured representation in xDS classes.

github-actions · 2024-01-26T02:12:31Z

🔍 Build Scan® (commit: `08675fe`)

Job name	Status	Build Scan®
build-windows-latest-jdk-19	✅	https://ge.armeria.dev/s/iumyfoj55dfwk
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/2oqafuttc4sgu
build-self-hosted-unsafe-jdk-19-snapshot-blockhound	❌ (failure)	https://ge.armeria.dev/s/otfbpyxukosmk
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/vqp6te2f5gsd6
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/u55ev2kqoqgn4
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/m4b5ik6ttsbjw
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/s2nxdnzh5m3oa
build-macos-12-jdk-19	✅	https://ge.armeria.dev/s/x3go4hxcizcnw

ikhoon

The overall direction looks good.

I prefer taking the Envoy message types as the type parameters instead of capsuled ones. If the call-site injects the innermost type, the container type can be wrapped at use-site.

ikhoon · 2024-01-26T06:32:18Z

xds/src/main/java/com/linecorp/armeria/xds/ResourceHolder.java

 */
 @UnstableApi
-public interface ResourceHolder {
+public interface ResourceHolder<T> {


Suggested change

public interface ResourceHolder<T> {

public interface ResourceHolder<T extends Message> {

ikhoon · 2024-01-26T06:47:12Z

xds/src/main/java/com/linecorp/armeria/xds/ResourceParser.java

@@ -28,17 +28,17 @@

 import com.linecorp.armeria.common.annotation.Nullable;

-abstract class ResourceParser {
+abstract class ResourceParser<T extends ResourceHolder<?>> {


It is still not type-safe. What do you think of taking Envoy Message types as the parameter?

abstract class ResourceParser<T extends Message> { @Nullable abstract String name(T message); abstract Class<T> clazz(); abstract ResourceHolder<T> parse(T message); ParsedResourcesHolder<T> parseResources(List<Any> resources) { ... } } final class ClusterResourceParser extends ResourceParser<Cluster> { ... }

Well the T (ResourceHolder) is the same type that XdsStreamSubscriber uses.

armeria/xds/src/main/java/com/linecorp/armeria/xds/DefaultResponseHandler.java

Lines 53 to 59 in 012b9d1

final XdsStreamSubscriber<T> subscriber = entry.getValue();

if (holder.parsedResources().containsKey(resourceName)) {

// Happy path: the resource updated successfully. Notify the watchers of the update.

subscriber.onData(holder.parsedResources().get(resourceName));

continue;

}

Let me try adding one more type to ResourceParser then.

Ah... Let me check first before adding another type.

Should we change the type of onData() instead?

class XdsStreamSubscriber<T extends Message> implements SafeCloseable { void onData(ResourceHolder<T> data) { ... } }

Ditto I'm not sure if it's a good idea to use ResourceHolder<T> instead of its subclasses because some of the API has its own methods. e.g. (ListenerResourceHolder.connectionManager(), ClusterResourceHolder.upstreamTlsContext())
We should cast them once more.

ikhoon · 2024-01-26T07:02:23Z

xds/src/main/java/com/linecorp/armeria/xds/ResourceHolderWithPrimer.java

+abstract class ResourceHolderWithPrimer
+        <SELF extends ResourceHolderWithPrimer<SELF, T, U>, T, U extends ResourceHolder<?>>
+        implements ResourceHolder<T> {
+
+    abstract SELF withPrimer(@Nullable U primer);
+
+    @Nullable
+    abstract U primer();


How about taking the resource type as is? It would make the type signature simpler.

Suggested change

abstract class ResourceHolderWithPrimer

<SELF extends ResourceHolderWithPrimer<SELF, T, U>, T, U extends ResourceHolder<?>>

implements ResourceHolder<T> {

abstract SELF withPrimer(@Nullable U primer);

@Nullable

abstract U primer();

abstract class ResourceHolderWithPrimer

<SELF extends ResourceHolderWithPrimer<SELF, T, U>, T extends Message, U extends Message>

implements ResourceHolder<T> {

abstract SELF withPrimer(@Nullable ResourceHolder<U> primer);

@Nullable

abstract ResourceHolder<U> primer();

We also need to change the concrete types with parameterized ones.

public final class RouteResourceHolder extends ResourceHolderWithPrimer<RouteResourceHolder, RouteConfiguration, Listener> { RouteResourceHolder(RouteConfiguration routeConfiguration, ResourceHolder<Listener> primer) { this.routeConfiguration = routeConfiguration; this.primer = primer; } ... @Override RouteResourceHolder withPrimer(@Nullable ResourceHolder<Listener> primer) { if (primer == null) { return this; } return new RouteResourceHolder(routeConfiguration, primer); } @Override @Nullable ResourceHolder<Listener> primer() { return primer; }

ikhoon · 2024-01-26T07:04:16Z

xds/src/main/java/com/linecorp/armeria/xds/AbstractResourceNode.java

@@ -24,7 +24,7 @@
 import io.envoyproxy.envoy.config.core.v3.ConfigSource;
 import io.grpc.Status;

-abstract class AbstractResourceNode<T> implements ResourceNode<AbstractResourceHolder> {
+abstract class AbstractResourceNode<T extends ResourceHolder<?>> implements ResourceNode<T> {


How about?

Suggested change

abstract class AbstractResourceNode<T extends ResourceHolder<?>> implements ResourceNode<T> {

abstract class AbstractResourceNode<T extends Message> implements ResourceNode<ResourceHolder<T>> {

If so, we should cast in some cases because we cannot use the concrete type.
e.g.
Can't use ClusterResourceHolder when creating ClusterSnapshot. Instead we should use ResourceHolder<Cluster>.

armeria/xds/src/main/java/com/linecorp/armeria/xds/ClusterResourceNode.java

Lines 91 to 99 in 012b9d1

final ClusterResourceHolder current = currentResourceHolder();

if (current == null) {

return;

}

if (!Objects.equals(newSnapshot.holder().primer(), current)) {

return;

}

parentWatcher.snapshotUpdated(

new ClusterSnapshot(current, newSnapshot, virtualHost, route, index));

Some of the holders have its own API:

armeria/xds/src/main/java/com/linecorp/armeria/xds/ConfigSourceClient.java

Line 70 in 012b9d1

final UpstreamTlsContext tlsContext = clusterSnapshot.holder().upstreamTlsContext();

ikhoon · 2024-01-26T07:10:14Z

xds/src/main/java/com/linecorp/armeria/xds/ParsedResourcesHolder.java

+final class ParsedResourcesHolder<T> {
+    private final Map<String, T> parsedResources;


What do you think of adding more constrains to the type parameter?

Suggested change

final class ParsedResourcesHolder<T> {

private final Map<String, T> parsedResources;

final class ParsedResourcesHolder<T extends Message> {

private final Map<String, ResourceHolder<T>> parsedResources;

codecov · 2024-01-29T09:04:42Z

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (fbcd997) 0.00% compared to head (08675fe) 73.94%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
.../com/linecorp/armeria/xds/ClusterResourceNode.java	63.63%	3 Missing and 1 partial ⚠️
...com/linecorp/armeria/xds/ListenerResourceNode.java	75.00%	2 Missing and 1 partial ⚠️
...va/com/linecorp/armeria/xds/RouteResourceNode.java	62.50%	2 Missing and 1 partial ⚠️
.../com/linecorp/armeria/xds/StaticResourceUtils.java	66.66%	3 Missing ⚠️
...java/com/linecorp/armeria/xds/ClusterSnapshot.java	60.00%	2 Missing ⚠️
...rp/armeria/xds/AbstractResourceNodeWithPrimer.java	85.71%	0 Missing and 1 partial ⚠️
.../com/linecorp/armeria/xds/EndpointXdsResource.java	85.71%	0 Missing and 1 partial ⚠️
...ava/com/linecorp/armeria/xds/RouteXdsResource.java	75.00%	1 Missing ⚠️
...ava/com/linecorp/armeria/xds/XdsBootstrapImpl.java	66.66%	1 Missing ⚠️
.../com/linecorp/armeria/xds/XdsStreamSubscriber.java	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #5410       +/-   ##
===========================================
+ Coverage        0   73.94%   +73.94%     
- Complexity      0    20690    +20690     
===========================================
  Files           0     1792     +1792     
  Lines           0    76321    +76321     
  Branches        0     9720     +9720     
===========================================
+ Hits            0    56439    +56439     
- Misses          0    15274    +15274     
- Partials        0     4608     +4608

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ikhoon

Looks clean! 👍

ikhoon · 2024-01-29T09:55:55Z

xds/src/main/java/com/linecorp/armeria/xds/ResourceParser.java

@@ -28,24 +28,24 @@

 import com.linecorp.armeria.common.annotation.Nullable;

-abstract class ResourceParser {
+abstract class ResourceParser<T extends XdsResource, U extends Message> {


To explicitly express the relationship of the two parameters.

Suggested change

abstract class ResourceParser<T extends XdsResource, U extends Message> {

abstract class ResourceParser<I extends Message, O extends XdsResource> {

That's a good idea. 👍

jrhee17

Looks good in terms of correctness. Thanks @minwoox 🙇 👍 🙇

jrhee17 · 2024-01-29T08:07:52Z

xds/src/main/java/com/linecorp/armeria/xds/AbstractResourceNodeWithPrimer.java

+
+    AbstractResourceNodeWithPrimer(XdsBootstrapImpl xdsBootstrap, @Nullable ConfigSource configSource,
+                                   XdsType type, String resourceName, @Nullable ResourceHolder<U> primer,
+                                   SnapshotWatcher<?> parentWatcher, ResourceNodeType resourceNodeType) {


If we're going to try to make everything type-safe, shouldn't parentWatcher also be handled?

Suggested change

SnapshotWatcher<?> parentWatcher, ResourceNodeType resourceNodeType) {

SnapshotWatcher<? extends Snapshot<T>> parentWatcher, ResourceNodeType resourceNodeType) {

Because the type is not used in this class and its parent class, I didn't add it. But let me add it for clarity. 😉

xds/src/main/java/com/linecorp/armeria/xds/XdsResource.java

jrhee17 · 2024-01-29T09:45:55Z

xds/src/main/java/com/linecorp/armeria/xds/SubscriberStorage.java

@@ -79,15 +82,20 @@ boolean unregister(XdsType type, String resourceName, ResourceWatcher<AbstractRe
        return false;
    }

-    Map<String, XdsStreamSubscriber> subscribers(XdsType type) {
-        return subscriberMap.getOrDefault(type, Collections.emptyMap());
+    <T extends XdsResource> Map<String, XdsStreamSubscriber<T>> subscribers(XdsType type) {


Question) Although it may seem like the appropriate parser, watcher, and callbacks is called, I don't think the addition of the generic parameter helps with type-safety since adding and fetching subscribers is force-cast anyways.

May I ask how the added generic parameter helps for XdsStreamSubscriber and XdsResourceParser helps?

What do you think of just adding the type parameter for ResourceHolder only if you think it helps with readability?

That's a good question. In fact, this was the most challenging decision I had to make during the implementation. I considered creating four different subclasses of the XdsStreamSubscriber to avoid the unsafe cast, but I felt it might be excessive.

Although it may seem like the appropriate parser, watcher, and callbacks is called, I don't think the addition of the generic parameter helps with type-safety since adding and fetching subscribers is force-cast anyways.

As you mentioned, the appropriate parser is used here, and by checking it in just one place, we can ensure the safety of this type cast. If this change is implemented, we can be confident that all other occurrences are also safe. Without this modification, we would need to verify multiple places. I hope that clarifies the rationale.

ikhoon · 2024-01-30T02:58:03Z

Thanks for cleaning up!

minwoox added the cleanup label Jan 26, 2024

minwoox added this to the 1.27.0 milestone Jan 26, 2024

minwoox requested review from ikhoon, jrhee17 and trustin as code owners January 26, 2024 01:01

ikhoon reviewed Jan 26, 2024

View reviewed changes

minwoox added 3 commits January 29, 2024 09:26

Address some of the comments from @ikhoon

ef296b1

Add unstable api

270639e

Address comments from @ikhoon

0ff56c0

Change type prameter of ResourceWatcher and ResourceNode

80b3f0c

ikhoon approved these changes Jan 29, 2024

View reviewed changes

jrhee17 approved these changes Jan 29, 2024

View reviewed changes

minwoox added 2 commits January 29, 2024 20:11

Address the comment from @jrhee17

af3e49f

Address the comment from @ikhoon

08675fe

ikhoon merged commit e494846 into line:main Jan 30, 2024
16 of 17 checks passed

minwoox deleted the xds_type_parameter branch January 30, 2024 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce type parameters where applicable for xDS classes. #5410

Introduce type parameters where applicable for xDS classes. #5410

minwoox commented Jan 26, 2024

github-actions bot commented Jan 26, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Jan 26, 2024

ikhoon Jan 26, 2024 •

edited

Loading

minwoox Jan 26, 2024

ikhoon Jan 26, 2024

ikhoon Jan 26, 2024 •

edited

Loading

minwoox Jan 26, 2024 •

edited

Loading

ikhoon Jan 26, 2024

ikhoon Jan 26, 2024

minwoox Jan 26, 2024

ikhoon Jan 26, 2024

codecov bot commented Jan 29, 2024 •

edited

Loading

ikhoon left a comment

ikhoon Jan 29, 2024

minwoox Jan 29, 2024

jrhee17 left a comment

jrhee17 Jan 29, 2024

minwoox Jan 29, 2024

jrhee17 Jan 29, 2024

minwoox Jan 29, 2024

ikhoon commented Jan 30, 2024

	public interface ResourceHolder<T> {
	public interface ResourceHolder<T extends Message> {

	final XdsStreamSubscriber<T> subscriber = entry.getValue();

	if (holder.parsedResources().containsKey(resourceName)) {
	// Happy path: the resource updated successfully. Notify the watchers of the update.
	subscriber.onData(holder.parsedResources().get(resourceName));
	continue;
	}

	abstract class AbstractResourceNode<T extends ResourceHolder<?>> implements ResourceNode<T> {
	abstract class AbstractResourceNode<T extends Message> implements ResourceNode<ResourceHolder<T>> {

	final ClusterResourceHolder current = currentResourceHolder();
	if (current == null) {
	return;
	}
	if (!Objects.equals(newSnapshot.holder().primer(), current)) {
	return;
	}
	parentWatcher.snapshotUpdated(
	new ClusterSnapshot(current, newSnapshot, virtualHost, route, index));

		final class ParsedResourcesHolder<T> {
		private final Map<String, T> parsedResources;

	abstract class ResourceParser<T extends XdsResource, U extends Message> {
	abstract class ResourceParser<I extends Message, O extends XdsResource> {

	SnapshotWatcher<?> parentWatcher, ResourceNodeType resourceNodeType) {
	SnapshotWatcher<? extends Snapshot<T>> parentWatcher, ResourceNodeType resourceNodeType) {

Introduce type parameters where applicable for xDS classes. #5410

Introduce type parameters where applicable for xDS classes. #5410

Conversation

minwoox commented Jan 26, 2024

github-actions bot commented Jan 26, 2024 • edited Loading

🔍 Build Scan® (commit: 08675fe)

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

minwoox Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 29, 2024 • edited Loading

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon commented Jan 30, 2024

github-actions bot commented Jan 26, 2024 •

edited

Loading

🔍 Build Scan® (commit: `08675fe`)

ikhoon Jan 26, 2024 •

edited

Loading

ikhoon Jan 26, 2024 •

edited

Loading

minwoox Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 29, 2024 •

edited

Loading