check topic existence before listings its subscriptions (#1733)

* check topic existence before listings its subscriptions * fix checkstyle * remove console building from checkstyle
allegro · Oct 26, 2023 · c8bff9a · c8bff9a
1 parent 9c4a7c1
commit c8bff9a
Show file tree

Hide file tree

Showing 9 changed files with 75 additions and 32 deletions.
diff --git a/.github/workflows/checkstyle.yml b/.github/workflows/checkstyle.yml
@@ -22,7 +22,8 @@ jobs:
           java-version: 17
           distribution: 'temurin'
       - name: Run check style
-        run: ./gradlew --continue clean checkstyleMain checkstyleTest checkstyleIntegration checkstyleJmh -PmaxCheckstyleWarnings=0
+        # ignore lengthy console setup tasks
+        run: ./gradlew --continue clean checkstyleMain checkstyleTest checkstyleIntegration checkstyleJmh -PmaxCheckstyleWarnings=0 -x attachHermesConsole -x prepareIndexTemplate
       - name: Run reviewdog
         if: ${{ success() || failure() }}
         env:

diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/Stats.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/Stats.java
@@ -10,7 +10,10 @@ public class Stats {
     private final SubscriptionStats subscriptionStats;
 
     @JsonCreator
-    public Stats(@JsonProperty("topicStats") TopicStats topicStats, @JsonProperty("subscriptionStats") SubscriptionStats subscriptionStats) {
+    public Stats(
+            @JsonProperty("topicStats")
+            TopicStats topicStats,
+            @JsonProperty("subscriptionStats") SubscriptionStats subscriptionStats) {
         this.topicStats = topicStats;
         this.subscriptionStats = subscriptionStats;
     }
@@ -25,8 +28,12 @@ public SubscriptionStats getSubscriptionStats() {
 
     @Override
     public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
         Stats stats = (Stats) o;
         return Objects.equals(topicStats, stats.topicStats) && Objects.equals(subscriptionStats, stats.subscriptionStats);
     }
@@ -38,9 +45,9 @@ public int hashCode() {
 
     @Override
     public String toString() {
-        return "Stats{" +
-                "topicStats=" + topicStats +
-                ", subscriptionStats=" + subscriptionStats +
-                '}';
+        return "Stats{"
+                + "topicStats=" + topicStats
+                + ", subscriptionStats=" + subscriptionStats
+                + '}';
     }
 }
diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/SubscriptionStats.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/SubscriptionStats.java
@@ -35,10 +35,16 @@ public long getAvroSubscriptionCount() {
 
     @Override
     public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
         SubscriptionStats that = (SubscriptionStats) o;
-        return subscriptionCount == that.subscriptionCount && trackingEnabledSubscriptionCount == that.trackingEnabledSubscriptionCount && avroSubscriptionCount == that.avroSubscriptionCount;
+        return subscriptionCount == that.subscriptionCount
+                && trackingEnabledSubscriptionCount == that.trackingEnabledSubscriptionCount
+                && avroSubscriptionCount == that.avroSubscriptionCount;
     }
 
     @Override
@@ -48,10 +54,10 @@ public int hashCode() {
 
     @Override
     public String toString() {
-        return "SubscriptionStats{" +
-                "subscriptionCount=" + subscriptionCount +
-                ", trackingEnabledSubscriptionCount=" + trackingEnabledSubscriptionCount +
-                ", avroSubscriptionCount=" + avroSubscriptionCount +
-                '}';
+        return "SubscriptionStats{"
+                + "subscriptionCount=" + subscriptionCount
+                + ", trackingEnabledSubscriptionCount=" + trackingEnabledSubscriptionCount
+                + ", avroSubscriptionCount=" + avroSubscriptionCount
+                + '}';
     }
 }
diff --git a/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicStats.java b/hermes-api/src/main/java/pl/allegro/tech/hermes/api/TopicStats.java
@@ -24,10 +24,17 @@ public TopicStats(@JsonProperty("topicCount") long topicCount,
 
     @Override
     public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
         TopicStats that = (TopicStats) o;
-        return topicCount == that.topicCount && ackAllTopicCount == that.ackAllTopicCount && trackingEnabledTopicCount == that.trackingEnabledTopicCount && avroTopicCount == that.avroTopicCount;
+        return topicCount == that.topicCount
+                && ackAllTopicCount == that.ackAllTopicCount
+                && trackingEnabledTopicCount == that.trackingEnabledTopicCount
+                && avroTopicCount == that.avroTopicCount;
     }
 
     @Override
@@ -37,12 +44,12 @@ public int hashCode() {
 
     @Override
     public String toString() {
-        return "TopicStats{" +
-                "topicCount=" + topicCount +
-                ", ackAllTopicCount=" + ackAllTopicCount +
-                ", trackingEnabledTopicCount=" + trackingEnabledTopicCount +
-                ", avroTopicCount=" + avroTopicCount +
-                '}';
+        return "TopicStats{"
+                + "topicCount=" + topicCount
+                + ", ackAllTopicCount=" + ackAllTopicCount
+                + ", trackingEnabledTopicCount=" + trackingEnabledTopicCount
+                + ", avroTopicCount=" + avroTopicCount
+                + '}';
     }
 
     public long getTopicCount() {

diff --git a/...java/pl/allegro/tech/hermes/infrastructure/zookeeper/ZookeeperSubscriptionRepository.java b/...java/pl/allegro/tech/hermes/infrastructure/zookeeper/ZookeeperSubscriptionRepository.java
@@ -131,6 +131,8 @@ public List<Subscription> getSubscriptionDetails(Collection<SubscriptionName> su
 
     @Override
     public List<String> listSubscriptionNames(TopicName topicName) {
+        topicRepository.ensureTopicExists(topicName);
+
         return childrenOf(paths.subscriptionsPath(topicName));
     }
 

diff --git a/...anagement/src/main/java/pl/allegro/tech/hermes/management/domain/MetricsDashboardUrl.java b/...anagement/src/main/java/pl/allegro/tech/hermes/management/domain/MetricsDashboardUrl.java
@@ -20,8 +20,13 @@ public String getUrl() {
 
     @Override
     public boolean equals(Object o) {
-        if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (this == o) {
+            return true;
+        }
+
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
 
         MetricsDashboardUrl that = (MetricsDashboardUrl) o;
 

diff --git a/.../main/java/pl/allegro/tech/hermes/management/domain/consistency/DcConsistencyService.java b/.../main/java/pl/allegro/tech/hermes/management/domain/consistency/DcConsistencyService.java
@@ -16,6 +16,7 @@
 import pl.allegro.tech.hermes.domain.group.GroupNotExistsException;
 import pl.allegro.tech.hermes.domain.group.GroupRepository;
 import pl.allegro.tech.hermes.domain.subscription.SubscriptionRepository;
+import pl.allegro.tech.hermes.domain.topic.TopicNotExistsException;
 import pl.allegro.tech.hermes.domain.topic.TopicRepository;
 import pl.allegro.tech.hermes.management.config.ConsistencyCheckerProperties;
 import pl.allegro.tech.hermes.management.domain.dc.DatacenterBoundRepositoryHolder;
@@ -138,13 +139,21 @@ private List<MetadataCopies> listCopiesOfSubscriptionsFromTopic(String topic) {
         Map<String, Future<List<Subscription>>> futuresPerDatacenter = new HashMap<>();
         for (DatacenterBoundRepositoryHolder<SubscriptionRepository> repositoryHolder : subscriptionRepositories) {
             Future<List<Subscription>> future = executor.submit(
-                    () -> repositoryHolder.getRepository().listSubscriptions(TopicName.fromQualifiedName(topic))
+                    () -> listSubscriptions(repositoryHolder.getRepository(), topic)
             );
             futuresPerDatacenter.put(repositoryHolder.getDatacenterName(), future);
         }
         return listCopies(futuresPerDatacenter, subscription -> subscription.getQualifiedName().getQualifiedName());
     }
 
+    private List<Subscription> listSubscriptions(SubscriptionRepository subscriptionRepository, String topic) {
+        try {
+            return subscriptionRepository.listSubscriptions(TopicName.fromQualifiedName(topic));
+        } catch (TopicNotExistsException e) {
+            return emptyList();
+        }
+    }
+
     private <T> List<MetadataCopies> listCopies(Map<String, Future<List<T>>> futuresPerDatacenter, Function<T, String> idResolver) {
         Map<String, MetadataCopies> copiesPerId = new HashMap<>();
         Set<String> datacenters = futuresPerDatacenter.keySet();

diff --git a/...n/java/pl/allegro/tech/hermes/management/infrastructure/console/FrontendRoutesFilter.java b/...n/java/pl/allegro/tech/hermes/management/infrastructure/console/FrontendRoutesFilter.java
@@ -13,7 +13,10 @@ public class FrontendRoutesFilter extends OncePerRequestFilter {
     private final String frontendEndpoint = "/";
 
     @Override
-    protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
+    protected void doFilterInternal(
+            HttpServletRequest request,
+            HttpServletResponse response,
+            FilterChain filterChain) throws ServletException, IOException {
         if (request.getRequestURI().startsWith("/ui")) {
             RequestDispatcher rd = request.getRequestDispatcher(frontendEndpoint);
             rd.forward(request, response);

diff --git a/...gration/src/integration/java/pl/allegro/tech/hermes/integration/management/StatsTest.java b/...gration/src/integration/java/pl/allegro/tech/hermes/integration/management/StatsTest.java
@@ -35,9 +35,12 @@ public void shouldGetStats() {
         TopicWithSchema topic2 = operations.createTopic(topic(group, name(), Topic.Ack.LEADER, ContentType.JSON, false));
         operations.createTopic(topic(group, name(), Topic.Ack.ALL, ContentType.JSON, true));
 
-        operations.createSubscription(topic1.getTopic(), subscription(topic1.getName(), name(), ContentType.AVRO, TrackingMode.TRACKING_OFF));
-        operations.createSubscription(topic2.getTopic(), subscription(topic2.getName(), name(), ContentType.JSON, TrackingMode.TRACK_ALL));
-        operations.createSubscription(topic2.getTopic(), subscription(topic2.getName(), name(), ContentType.AVRO, TrackingMode.TRACKING_OFF));
+        operations.createSubscription(topic1.getTopic(),
+                subscription(topic1.getName(), name(), ContentType.AVRO, TrackingMode.TRACKING_OFF));
+        operations.createSubscription(topic2.getTopic(),
+                subscription(topic2.getName(), name(), ContentType.JSON, TrackingMode.TRACK_ALL));
+        operations.createSubscription(topic2.getTopic(),
+                subscription(topic2.getName(), name(), ContentType.AVRO, TrackingMode.TRACKING_OFF));
 
         // when then
         Assertions.assertThat(management.statsEndpoint().getStats()).isEqualTo(new Stats(