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

[ISSUE #4834] Add protocol exclusive fields filter for /v2/configuration endpoint #4835

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

Pil0tXia
Copy link
Member

Fixes #4834

Motivation

Since EventMeshTCPConfiguration, EventMeshHTTPConfiguration and EventMeshGrpcConfiguration are subclasses of CommonConfiguration, their serialized JsonString would contain superclass fields, which enlarges the response body and brings in difficulty in getting CommonConfiguration fields.

Modifications

  • Add a PropertyFilter to remove superclass fields from the JsonString of EventMeshTCPConfiguration, EventMeshHTTPConfiguration and EventMeshGrpcConfiguration, then add CommonConfiguration into the response body.
  • Add configs param which accepts two values, exclusive and all.
  • Add unified exception handling
  • The actual version number will be returned after merging [ISSUE #4052] Implement the functionality of EventMeshVersion class.  #4055.

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (JavaDocs)

@Pil0tXia Pil0tXia added the ready for review PR is waiting for reviewer's approval or opinion (used as a strong reminder) label Apr 27, 2024
@Pil0tXia Pil0tXia requested a review from mxsm April 27, 2024 10:55
@xwm1992
Copy link
Contributor

xwm1992 commented Apr 28, 2024

I don’t understand the purpose of your PR modification. Why do you want to filter out the fields of the parent class? The fields of the parent class should be some common fields, and the subclass is an extension. Why do you want to filter out the common fields?
@Pil0tXia

@Pil0tXia
Copy link
Member Author

The EventMesh Dashboard needs to retrieve the configuration of each EventMesh Runtime after it is connected to the cluster, store it in the database for display on the frontend pages.

Since EventMeshTCPConfiguration, EventMeshHTTPConfiguration, and EventMeshGrpcConfiguration are subclasses of CommonConfiguration, the current master branch's /v2/configuration admin endpoint will return redundant fields of CommonConfiguration in the configurations for TCP, HTTP, and GRPC. This leads to the following issues:

  1. When displaying configurations on the frontend pages of EventMesh Dashboard, three copy of CommonConfiguration fields will be shown, wasting space and potentially confusing users.
  2. EventMesh Dashboard needs to support maintaining multiple versions of EventMesh clusters. If EventMesh Dashboard removes duplicate fields, it needs to know which fields come from CommonConfiguration. If EventMesh Dashboard maintains a CommonConfiguration itself, but different versions of EventMesh Runtime have different CommonConfiguration, then EventMesh Dashboard will have multiple CommonConfiguration classes.
  3. If EventMesh Runtime does not enable all three protocols TCP, HTTP, GRPC, and instead only enables two or one of them, EventMesh Dashboard needs to iterate over TCP, HTTP, GRPC to retrieve the values of this field when storing the fields of CommonConfiguration in the database.

These issues could have been avoided, eliminating the display of redundant content, avoiding the need to maintain the CommonConfiguration class on the EventMesh Dashboard side, and avoiding complex processing logic. This PR extracts the member fields of the CommonConfiguration class into the commonConfiguration JSON field to assist EventMesh Dashboard in distinguishing fields from CommonConfiguration.

Callers of the /v2/configuration admin endpoint can also specify the configs parameter as all, keeping the fields of CommonConfiguration in the configurations for TCP, HTTP, and GRPC.

In the future, we may consider integrating the metaStorage Plugin with ConfigService to report configurations to the registry. When we need this functionality, I can start working on this feature.


EventMesh Dashboard需要在接入集群后,获取每一台EventMesh Runtime的配置,存入数据库中,以供前端页面上展示。

由于EventMeshTCPConfigurationEventMeshHTTPConfigurationEventMeshGrpcConfigurationCommonConfiguration的子类,因此,目前master分支中/v2/configuration管理端点将会在TCP,HTTP,GRPC的配置中都返回一份CommonConfiguration的成员字段,存在冗余。这会带来以下问题:

  1. EventMesh Dashboard在前端页面上显示配置时,将会显示三份一模一样的CommonConfiguration,浪费空间,容易使用户迷惑。
  2. EventMesh Dashboard需要支持同时维护多个版本的EventMesh集群。如果由EventMesh Dashboard来移除重复的字段,那么EventMesh Dashboard需要知道有哪些字段来自CommonConfiguration。如果EventMesh Dashboard自行维护一个CommonConfiguration,但是不同版本的EventMesh Runtime具有不同的CommonConfiguration,那么EventMesh Dashboard就会存在多个CommonConfiguration
  3. 如果EventMesh Runtime没有开启TCP,HTTP,GRPC全部三种协议,而是只开启了其中两种或一种,则EventMesh Dashboard在将CommonConfiguration的字段存入数据库时,需要依次遍历TCP,HTTP,GRPC,获取此字段的值。

这些问题本可以被避免,不用显示重复内容,不需要在EventMesh Dashboard侧维护CommonConfiguration类,不需要复杂的处理逻辑。本PR将CommonConfiguration类中的成员字段提取到commonConfigurationJSON字段下,以帮助EventMesh Dashboard分辨来自CommonConfiguration的字段。

/v2/configuration管理端点的调用者也可以指定configs参数为all,在返回结果中保留TCP,HTTP,GRPC三种协议的配置类中CommonConfiguration的字段。

在未来,我们可能会考虑将metaStorage Plugin与ConfigService结合,将配置上报到注册中心。当我们需要此功能时,我可以着手做这个特性。

Copy link
Member

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@Pil0tXia Pil0tXia merged commit 70f9892 into apache:master Apr 29, 2024
9 checks passed
@Pil0tXia Pil0tXia deleted the pil0txia/admin_4834 branch April 29, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is waiting for reviewer's approval or opinion (used as a strong reminder)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add protocol exclusive fields filter for /v2/configuration endpoint
4 participants