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.

Sign up for GitHub

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

Jump to bottom

[ISSUE #73] Add remoting service #74

Merged
merged 5 commits into from
Mar 29, 2024
Merged

[ISSUE #73] Add remoting service #74

merged 5 commits into from
Mar 29, 2024

Conversation

Lambert-Rao
Copy link
Contributor

No description provided.

Pil0tXia
Pil0tXia reviewed Mar 28, 2024
View reviewed changes
...h-dashboard-common/src/main/java/org/apache/eventmesh/dashboard/common/enums/DataStatus.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/apache/eventmesh/dashboard/common/enums/metadata/MetadataTypeEnum.java Outdated Show resolved Hide resolved
.../main/java/org/apache/eventmesh/dashboard/common/enums/metadata/MetadataServiceTypeEnum.java Outdated Show resolved Hide resolved
.../main/java/org/apache/eventmesh/dashboard/common/enums/metadata/MetadataServiceTypeEnum.java Outdated Show resolved Hide resolved
...mon/src/main/java/org/apache/eventmesh/dashboard/common/enums/metadata/MetadataTypeEnum.java Outdated Show resolved Hide resolved
...rc/main/java/org/apache/eventmesh/dashboard/common/model/remoting/acl/CreateAclResponse.java
Comment on lines +20 to +23
public class CreateAclResponse {

Boolean success;
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between Response and Result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

result have a future of response

...n/src/main/java/org/apache/eventmesh/dashboard/common/model/remoting/acl/GetAclsRequest.java Show resolved Hide resolved
...c/main/java/org/apache/eventmesh/dashboard/common/model/remoting/client/GetClientResult.java
Comment on lines +22 to +25
public class GetClientResult {

CompletableFuture<GetClientResponse> getClientResponseFuture;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to warp CompletableFuture<GetClientResponse> into GetClientResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for a clearer division of responsibilities

.../main/java/org/apache/eventmesh/dashboard/common/model/remoting/config/GetConfigRequest.java Outdated
Comment on lines 35 to 36

private List<String> brokerAddress;
Copy link
Member

Choose a reason for hiding this comment

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

Should it be namesrvAddress?

...main/java/org/apache/eventmesh/dashboard/common/model/remoting/config/GetConfigResponse.java Outdated
Comment on lines 34 to 37

List<ConfigMetadata> configMetadataList;
private Boolean success;
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems that configMetadataList is missing a private decorator.

Pil0tXia
Pil0tXia reviewed Mar 29, 2024
View reviewed changes
.../main/java/org/apache/eventmesh/dashboard/common/model/remoting/offset/GetOffsetRequest.java Outdated
Comment on lines 25 to 28

private GroupMetadata groupMetadata;
private String bootStrapServers;

private String groupName;
Copy link
Member

Choose a reason for hiding this comment

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

It should be bootstrap instead of bootStrap as bootstrap is a single word.

Is there any relationship between GetOffsetRequest and these two members?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, what fields are needed for a request, developers who implement these remoting services can modify the fields

Copy link
Member

Choose a reason for hiding this comment

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

okay, then lets leave a TODO comment and delete these members.

Copy link
Member

Choose a reason for hiding this comment

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

You may finish this part in the next PR.

...ommon/src/main/java/org/apache/eventmesh/dashboard/common/model/metadata/MetadataConfig.java Outdated
Comment on lines 30 to 31
//center url
private String registryAddress;
Copy link
Member

Choose a reason for hiding this comment

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

center naming shall be replaced.

scwlkq and others added 3 commits March 29, 2024 16:08
@scwlkq @Lambert-Rao
[ISSUE apache#45] Implement methods from storage-plugin.admin(rocketm…
b9dbe22 
…q) (apache#66)

* modify RocketmqTopicCore, using SDKManager

* fix style

* delete some variable in RocketmqProperties and TopicProperties & remote additional endPoint name

* remove junit dependency back to console module

* remove unnecessary brackets & revert em-dashboard-pom.xml & move RocketmqProperties to core.dto

* add todo

* move todo to common sdk dependencies & fix style

* add test and junit dependencies in core pom.xml

* revert the indentation in pom.xml & move test related dependencies to bottom in core-pom.xml

* revert indentation in pom.xml

* revert indentation in pom.xml

* remove unnecessary params

* remove params

* remove empty line
@Lambert-Rao
chore: fix typos
f66418c
@Lambert-Rao
Merge branch 'dev' into feat_remoting_73
64e87ce
@Pil0tXia
Copy link
Member

You may finish the bootStrapServers naming in ResetOffsetRequest in the next PR.

@Pil0tXia Pil0tXia merged commit 13eae3f into apache:dev Mar 29, 2024
2 checks passed
SLSJL pushed a commit to SLSJL/eventmesh-dashboard that referenced this pull request Mar 31, 2024
Merge branch 'dev' of https://github.com/SLSJL/eventmesh-dashboard in…
e75f4ae 
…to dev

* 'dev' of https://github.com/SLSJL/eventmesh-dashboard:
  [ISSUE apache#73] Add remoting service (apache#74)
  [ISSUE apache#45] Implement methods from storage-plugin.admin(rocketmq) (apache#66)
  [ISSUE apache#71] Reduce log file size and Streamline debug output (apache#72)
  [ISSUE apache#69] Integrate database credentials in auto-deploy (apache#70)
  [ISSUE apache#67] Fix HealthCheckResultMapper which leads to application error
  [ISSUE apache#64] Support automated deployment and Fix runtime packaging errors (apache#65)
  [ISSUE apache#60] add SDK manager (apache#62)
  [ISSUE apache#57] Modify the field, synchronize the modification, and add the mapper method (apache#58)
  [ISSUE apache#29] Set up EventMesh Dashboard Front-end (apache#56)
  [ISSUE apache#49] RocketMQ and Nacos health check (apache#53)
  [ISSUE apache#51] Config Mgmt basic function and config,runtime,store,cluster SQL (apache#52)

# Conflicts:
#	eventmesh-dashboard-view/public/index.html
#	eventmesh-dashboard-view/src/App.tsx
#	eventmesh-dashboard-view/src/index.tsx
#	eventmesh-dashboard-view/src/routes/RootLayout.tsx
#	eventmesh-dashboard-view/src/routes/navigation/Navigation.tsx
#	eventmesh-dashboard-view/src/routes/navigation/NavigationItem.tsx
#	eventmesh-dashboard-view/src/routes/topic/Topic.tsx
#	eventmesh-dashboard-view/src/routes/topic/stats/AbnormalTopicCount.tsx
#	eventmesh-dashboard-view/src/routes/topic/stats/Stats.tsx
#	eventmesh-dashboard-view/src/routes/topic/stats/StatsChart.tsx
#	eventmesh-dashboard-view/src/routes/topic/stats/TopicCount.tsx
#	eventmesh-dashboard-view/src/routes/topic/topic-list/TopicList.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pil0tXia Pil0tXia Pil0tXia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Lambert-Rao @Pil0tXia @scwlkq