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

[Enhancement #3495] Add redis admin #4302

Closed
wants to merge 5 commits into from

Conversation

fabian4
Copy link
Member

@fabian4 fabian4 commented Jul 28, 2023

Fixes #3495.

Motivation

Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

  • Add redis-admin in eventmesh-admin
  • Add redisAdmin and redisAdminAdapter in redis-storage
  • Add rocketmqService in rocketmq-admin to create topic (which calls the function in rocketmq-storage

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #4302 (96cf918) into master (5b8616b) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

❗ Current head 96cf918 differs from pull request most recent head 38ba65d. Consider uploading reports for the commit 38ba65d to get more accurate results

@@             Coverage Diff              @@
##             master    #4302      +/-   ##
============================================
- Coverage     16.87%   16.85%   -0.03%     
  Complexity     1428     1428              
============================================
  Files           593      596       +3     
  Lines         26035    26087      +52     
  Branches       2366     2380      +14     
============================================
+ Hits           4394     4397       +3     
- Misses        21215    21263      +48     
- Partials        426      427       +1     
Files Changed Coverage Δ
...org/apache/eventmesh/admin/rocketmq/Constants.java 0.00% <ø> (ø)
...ventmesh/admin/rocketmq/handler/TopicsHandler.java 0.00% <0.00%> (ø)
...ntmesh/admin/rocketmq/service/RocketMQService.java 0.00% <0.00%> (ø)
...ache/eventmesh/storage/redis/admin/RedisAdmin.java 0.00% <0.00%> (ø)
...entmesh/storage/redis/admin/RedisAdminAdaptor.java 0.00% <0.00%> (ø)

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pandaapo
Copy link
Member

Some of your classes have almost the same content with classes in eventmesh-admin-rocketmq module. Could your consider doing extraction and refactoring work to avoid a large amount of duplicated code?
org.apache.eventmesh.admin.redis.Constants
org.apache.eventmesh.admin.redis.controller.AdminController
org.apache.eventmesh.admin.redis.handler.TopicsHandler
org.apache.eventmesh.admin.redis.request.TopicCreateRequest
org.apache.eventmesh.admin.redis.response.TopicResponse
org.apache.eventmesh.admin.redis.util.RequestMapping
org.apache.eventmesh.admin.redis.util.UrlMappingPattern
org.apache.eventmesh.admin.redis.service.RedisService -- org.apache.eventmesh.admin.rocketmq.service.RocketMQService

Additionally, org.apache.eventmesh.storage.redis.admin.RedisAdmin and org.apache.eventmesh.storage.redis.admin.RedisAdminAdaptor, these 2 are almost the same with the classes in eventmesh-storage-rocketmq module. RocketMQAdmin and RocketMQAdminAdaptor are not in eventmesh-admin module for now, but they will be. So maybe you can design class like template class or abstract class in advance, so that classes in other eventmesh-admin-xxx modules can reuse them in the future.

@fabian4
Copy link
Member Author

fabian4 commented Jul 28, 2023

I thought about it first. considering there is a task related #4040, I just simply added the redis-admin and submit the pr waiting for further disscusion.

I am still confused about the admin part in eventmesh-storage-plugin eventmesh-runtime and eventmesh-admin. I noticed there is a task related to re-design this part, so we can discuss it first to avoid confict.

@pandaapo
Copy link
Member

I just simply added the redis-admin and submit the pr waiting for further disscusion.

I am still confused about the admin part in eventmesh-storage-plugin eventmesh-runtime and eventmesh-admin.

Contributing code in this way is not good, both for you and for EventMesh. You should program only after fully understanding the relevant code you are dealing with, and verify the functionality of your code.

@fabian4
Copy link
Member Author

fabian4 commented Jul 30, 2023

I just simply added the redis-admin and submit the pr waiting for further disscusion.

I am still confused about the admin part in eventmesh-storage-plugin eventmesh-runtime and eventmesh-admin.

Contributing code in this way is not good, both for you and for EventMesh. You should program only after fully understanding the relevant code you are dealing with, and verify the functionality of your code.

I'm sorry for my thoughtless behavior in this pr and I will make a plan about it first the next time.

@fabian4 fabian4 closed this Jul 30, 2023
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.

[Enhancement] Optimize the eventmesh-admin of EventMesh
2 participants