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

Idle topic finder #751

Merged
merged 16 commits into from
Oct 15, 2022
Merged

Conversation

chinghongfang
Copy link
Collaborator

@chinghongfang chinghongfang commented Sep 23, 2022

#739
實做一個搜查 idle topics 的框架。方便未來使用 web API 取得我們定義的 idle topic 。

使用方法暫定為:
提供 duration ,找出 duration 時間內 "沒有新 record " 且 "沒有 consumer poll" 的 topic。

try (var finder = new IdleTopicFinder("localhost:9092")) {
  finder.idleTopics(Duration.ofSeconds(3));
}

@chia7712
Copy link
Contributor

@chinghongfang @harryteng9527 我們應該要先討論 APIs 的格式,不然有可能會需要額外的修改。

例如,我們已經有 Topic APIs 了,是否能在 GET /topics的搜尋條件加上idle=true來指定搜尋 idle topics 就好?

@chinghongfang
Copy link
Collaborator Author

在 GET /topics的搜尋條件加上idle=true來指定搜尋 idle topics

好主意!有想過要不要加在 /topics 底下,沒想到加在 query 內。我再移到 TopicsHandler 裡面。

@chinghongfang
Copy link
Collaborator Author

依據先前的 討論 ,這隻 PR 先實做 "找尋 idle topic 的邏輯",並且把判斷拉出 handler ,新增一個 class 。

使用方法為:
提供 duration ,找出 duration 時間內 "沒有新 record " 且 "沒有 consumer poll" 的 topic。

try (var finder = new IdleTopicFinder("localhost:9092")) {
  finder.idleTopics(Duration.ofSeconds(3));
}

@chinghongfang chinghongfang changed the title Idle topic handler Idle topic finder Sep 26, 2022
Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chinghongfang 感謝這個很棒的元件,有幾個設計面的建議請看一下

@chinghongfang
Copy link
Collaborator Author

因為 #833Admin 冠上 @Deprecated ,不清楚未來會不會將 Admin 淘汰掉?

所以這裡想請問 @chia7712 ,請問 idleTopic 的功能還需要加到 Admin 嘛?還是加到 AsyncAdmin 就好?

@chia7712
Copy link
Contributor

請問 idleTopic 的功能還需要加到 Admin 嘛?還是加到 AsyncAdmin 就好?

以未來性來講,放到 AsyncAdmin 會比較好,不過這樣實作/介面都要改成用非同步包裝

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chinghongfang 整體看起來很不錯,剩下一個建議請看一下

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chinghongfang 不好意思,還有兩個建議請看一下

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@chinghongfang 有幾個跟風格有關的建議,請看一下

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

LGTM

@chinghongfang chinghongfang merged commit 49a080b into opensource4you:main Oct 15, 2022
@chinghongfang chinghongfang deleted the idleTopics branch October 26, 2022 08:15
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.

3 participants