Refactor for mono #28

yazebochan · 2020-04-14T15:08:24Z

No description provided.

germanosin · 2020-04-14T15:20:04Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/service/MetricsUpdateService.java

-        log.debug("Start getting metrics for kafkaCluster: " + kafkaCluster.getName());
-        kafkaService.loadClusterMetrics(kafkaCluster);
-        zookeeperService.checkZookeeperStatus(kafkaCluster);
+        Mono.just(true)


looks strange, is it possible to avoid Mono.just here?

removed mono from this method

germanosin · 2020-04-14T15:20:40Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

-        kafkaCluster.getCluster().setStatus(ServerStatus.ONLINE);
-        loadMetrics(kafkaCluster);
-        loadTopicsData(kafkaCluster);
+        Mono.just(false)


What sense to use Mono just here?

removed redundant mono usage there

germanosin · 2020-04-14T15:21:34Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

-        }
-        kafkaCluster.setTopics(foundTopics);
+        ClusterUtil.toMono(adminClient.listTopics(listTopicsOptions).names())
+                .map(tl -> {


something wrong with idents

germanosin · 2020-04-14T15:22:45Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

                .values()
                .values()
                .iterator()
-                .next()
-                .get();
+                .next()).map(s -> Mono.just(true));


Looks dirty

germanosin · 2020-04-15T14:54:30Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

@@ -50,26 +47,26 @@ public void loadClusterMetrics(KafkaCluster kafkaCluster) {


    @SneakyThrows
-    public Mono<ResponseEntity<Topic>> createTopic(KafkaCluster cluster, Mono<TopicFormData> topicFormData) {
+    public Mono<ResponseEntity<Topic>> createTopic(AdminClient adminClient, KafkaCluster cluster, Mono<TopicFormData> topicFormData) {


let's remove ResponseEntity from service layer

germanosin · 2020-04-15T14:56:12Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

                .values()
                .values()
                .iterator()
-                .next()
-                .get();
+                .next()).subscribe();


Let's pass mono here to upper level

…for-mono

…o refactor-for-mono

germanosin · 2020-04-21T11:01:19Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/model/KafkaCluster.java


-    List<Topic> topics = new ArrayList<>();
-    private Map<String, TopicDetails> topicDetailsMap = new ConcurrentHashMap<>();
+    private List<Topic> topics = new ArrayList<>();


why this one is not final?

germanosin · 2020-04-21T11:05:09Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/service/ClusterService.java

@@ -59,14 +60,16 @@
    public Mono<ResponseEntity<Topic>> createTopic(String name, Mono<TopicFormData> topicFormData) {
        KafkaCluster cluster = clustersStorage.getClusterByName(name);


Services should not return response entity. This is controller responsibility.

germanosin · 2020-04-21T11:09:53Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/zookeeper/ZookeeperService.java

            return;
        }
-
        kafkaCluster.getBrokersMetrics().setZooKeeperStatus(ZooKeeperConstants.ONLINE);


Should be immutable

germanosin · 2020-04-21T11:24:43Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/service/MetricsUpdateService.java

+        log.debug("Start getting metrics for kafkaCluster: {}", clusterWithId.getKafkaCluster());
+        return kafkaService.getUpdatedCluster(clusterWithId)
+                .map(s -> {
+                    zookeeperService.checkZookeeperStatus(s.getKafkaCluster());


looks strange

germanosin · 2020-04-21T11:26:46Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                    ac ->
+                        getClusterMetrics(ac, kafkaCluster).flatMap(
+                                metrics -> {
+                                    Cluster cluster = kafkaCluster.getCluster();


you should create new instance of cluster

germanosin · 2020-04-21T11:29:07Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                                })
+            ).onErrorResume(
+                    e -> {
+                        Cluster cluster = kafkaCluster.getCluster();


you should create new one

germanosin · 2020-04-21T11:31:34Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+        listTopicsOptions.listInternal(true);
+        return ClusterUtil.toMono(adminClient.listTopics(listTopicsOptions).names())
+                    .map(tl -> {
+                    kafkaCluster.getCluster().setTopicCount(tl.size());


why do you mutate it?

germanosin · 2020-04-21T11:32:44Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                    }
+                return s;
+                })
+                .flatMap(td -> collectTopicData(cluster, adminClient, td))


germanosin · 2020-04-22T09:06:10Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/util/ClusterUtil.java

+    }
+
+    @SneakyThrows
+    public static <T> T clone(T subject) {


It's bad idea

germanosin · 2020-04-22T09:08:59Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                                brokersMetrics.setBrokerCount(metrics.getBrokerCount());
+                                brokersMetrics.activeControllers(metrics.getActiveControllers());
+                                brokersMetrics.brokerCount(metrics.getBrokerCount());
+                                resetMetrics(brokersMetrics);


better naming what you exactly reseting

germanosin · 2020-04-22T09:11:54Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+
+        topic.partitions(partitions);
+
+        if (kafkaCluster.getTopicDetailsMap() == null) {


better to collect on top level

germanosin · 2020-04-22T09:12:48Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                                        .flatMap(topics ->
+                                            loadTopicConfig(ac, topics.stream().map(InternalTopic::getName).collect(Collectors.toList())).collectList()
+                                                    .map(s -> s.stream().collect(Collectors.toMap(map -> new ArrayList<>(map.entrySet()).get(0).getKey(),
+                                                            e -> new ArrayList<>(e.entrySet()).get(0).getValue())))


germanosin · 2020-04-23T06:58:20Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/service/ClusterService.java

    }

    @SneakyThrows
-    public Mono<ResponseEntity<Flux<ConsumerGroup>>> getConsumerGroup (String clusterName) {
+    public Flux<ConsumerGroup> getConsumerGroup (String clusterName) {


getConsumerGroup -> getConsumerGroups

germanosin · 2020-04-23T06:58:51Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/cluster/model/KafkaCluster.java

-    Exception lastKafkaException;
-    Exception lastZookeeperException;
+@Builder(toBuilder = true, builderClassName = "KafkaClusterBuilder")
+public class KafkaCluster implements Serializable {


Why Serializable?

germanosin · 2020-04-23T07:27:39Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

                    NewTopic newTopic = new NewTopic(topicData.getName(), topicData.getPartitions(), topicData.getReplicationFactor().shortValue());
                    newTopic.configs(topicData.getConfigs());
-
                    createTopic(adminClient, newTopic);


you should return mono to next method

germanosin · 2020-04-23T07:34:29Z

kafka-ui-api/src/main/java/com/provectus/kafka/ui/kafka/KafkaService.java

+                    partitionDto.partition(partition.partition());
+                    partitionDto.inSyncReplicasCount(partition.isr().size());
+                    partitionDto.replicasCount(partition.replicas().size());
+                    replicas.addAll(partition.replicas().stream().map(


Replicas should be initialized per partition

germanosin · 2020-04-23T07:39:16Z

kafka-ui-contract/src/main/resources/swagger/kafka-ui-api.yaml

@@ -325,6 +325,16 @@ components:
          additionalProperties:
            type: string

+    Metrics:


Why is it here?

germanosin · 2020-04-23T07:40:07Z

kafka-ui-contract/pom.xml

@@ -58,6 +58,7 @@
                                        <modelPackage>com.provectus.kafka.ui.model</modelPackage>
                                        <apiPackage>com.provectus.kafka.ui.api</apiPackage>
                                        <sourceFolder>kafka-ui-contract</sourceFolder>
+                                        <serializableModel>true</serializableModel>


* Refactor for mono (#28) * starting refactor mono * topics and cluster refactored into mono way instead of get * fixed dirty mistakes * param * changed collecttopicdata method to correctly * refactored metrics to copyonwrite methoology * metrics params updated * fixed silly mistake * Let's think immutable * immutable * dumb mistakes fixed * changed to immutable * added new mappers * changed to immutable * typo was fixed * imports were cleared * Refactored * imports were optimized Co-authored-by: Roman Nedzvetskiy <[email protected]> Co-authored-by: German Osin <[email protected]> * need to refactor * issue-31/add-ui-prefix-for-react-routes (#34) * issue-31/add-ui-prefix-for-react-routes * added route for /ui path (#40) Co-authored-by: Roman Nedzvetskiy <[email protected]> Co-authored-by: Roman Nedzvetskiy <[email protected]> Co-authored-by: Roman Nedzvetskiy <[email protected]> * fix-bug-topic-not-creatable-when-no-custom-params (#37) * fix-bug-topic-not-creatable-when-no-custom-params * remove-linter-changes-to-cleanup-PR * Enhancement/improve time to retain usability v2 (#35) * enhancement/improve-time-to-retain-usability * add-btn-controls-for-time-to-retain-for-topics * updating of topic done properly * Added supportedCommands enum in cluster for getting correct method for topics settings update, changed request version from patch to put * AdminClient now boxed into extendedAdminClient * Redundant imports removed * SupportedFeatures method moved to util * Methods of updating cluster separated * Subsribed to change topics methods, replaced path of ts class * fix-routes-for-details (#45) * Added test and changed getAdminClient logic from clusterId to clusterName * Changed updating methods mono logic, added update cluster topic method * Fixed test by removing updateCluster calls, fixed add topic * Removed redundant injection and import * Added behaviour if topic wasn't created * Removed redundant imports * Dublicate lines in ClusterService moved to updateCluster method * CreateTopic method updated -- now returns topicName, update topic method changed -- removed redundant param clusterName Co-authored-by: Roman Nedzvetskiy <[email protected]> Co-authored-by: German Osin <[email protected]> Co-authored-by: Azat Gataullin <[email protected]>

* starting refactor mono * topics and cluster refactored into mono way instead of get * fixed dirty mistakes * param * changed collecttopicdata method to correctly * refactored metrics to copyonwrite methoology * metrics params updated * fixed silly mistake * Let's think immutable * immutable * dumb mistakes fixed * changed to immutable * added new mappers * changed to immutable * typo was fixed * imports were cleared * Refactored * imports were optimized Co-authored-by: Roman Nedzvetskiy <[email protected]> Co-authored-by: German Osin <[email protected]>

Roman Nedzvetskiy added 2 commits April 10, 2020 18:09

starting refactor mono

ca49eae

topics and cluster refactored into mono way instead of get

3f7b68f

yazebochan requested review from workshur and germanosin April 14, 2020 15:08

germanosin suggested changes Apr 14, 2020

View reviewed changes

Roman Nedzvetskiy added 3 commits April 14, 2020 20:07

fixed dirty mistakes

8ac0fed

param

100db33

changed collecttopicdata method to correctly

dad2b5e

germanosin suggested changes Apr 15, 2020

View reviewed changes

Roman Nedzvetskiy and others added 8 commits April 17, 2020 13:28

refactored metrics to copyonwrite methoology

a912a42

metrics params updated

771e09e

fixed silly mistake

f328423

Let's think immutable

41f0ccb

Merge branch 'master' of github.com:provectus/kafka-ui into refactor-…

c5b4a87

…for-mono

Merge branch 'refactor-for-mono' of github.com:provectus/kafka-ui int…

2f504c1

…o refactor-for-mono

immutable

9855413

dumb mistakes fixed

941e7f1

germanosin suggested changes Apr 21, 2020

View reviewed changes

changed to immutable

2c6cca6

germanosin suggested changes Apr 22, 2020

View reviewed changes

added new mappers

7d73096

germanosin suggested changes Apr 23, 2020

View reviewed changes

Roman Nedzvetskiy and others added 5 commits April 23, 2020 16:51

changed to immutable

ad58be2

typo was fixed

a633249

imports were cleared

7cc168b

Refactored

c1ee1cf

imports were optimized

0ff6fda

yazebochan merged commit b246577 into master Apr 24, 2020

workshur deleted the refactor-for-mono branch March 17, 2021 10:42

Donutellko pushed a commit to Donutellko/kafka-ui that referenced this pull request Jul 12, 2024

Infra: Fix permissions for checkout (provectus#28)

9186cd0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for mono #28

Refactor for mono #28

yazebochan commented Apr 14, 2020

germanosin Apr 14, 2020

yazebochan Apr 14, 2020

germanosin Apr 14, 2020

yazebochan Apr 14, 2020

germanosin Apr 14, 2020

yazebochan Apr 14, 2020

germanosin Apr 14, 2020

yazebochan Apr 14, 2020

germanosin Apr 15, 2020

germanosin Apr 15, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 21, 2020

germanosin Apr 22, 2020

germanosin Apr 22, 2020

germanosin Apr 22, 2020

germanosin Apr 22, 2020

germanosin Apr 23, 2020

germanosin Apr 23, 2020

germanosin Apr 23, 2020

germanosin Apr 23, 2020

germanosin Apr 23, 2020

germanosin Apr 23, 2020

		@@ -59,14 +60,16 @@
		public Mono<ResponseEntity<Topic>> createTopic(String name, Mono<TopicFormData> topicFormData) {
		KafkaCluster cluster = clustersStorage.getClusterByName(name);


		topic.partitions(partitions);

		if (kafkaCluster.getTopicDetailsMap() == null) {

Refactor for mono #28

Refactor for mono #28

Conversation

yazebochan commented Apr 14, 2020

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment