-
Notifications
You must be signed in to change notification settings - Fork 62
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
Implemetation of MBean client #15
Conversation
The example LGTM. Could you add it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this patch. a couple of comments below.
import java.net.MalformedURLException; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class MBeanClientTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to build a jmx server locally and test all getter by local MBean server (see https://docs.oracle.com/javase/7/docs/api/java/lang/management/ManagementFactory.html#getPlatformMBeanServer())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that thing exists, I will try it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
記得補上這邊提到的測試喔
quote the property value field. After this, special character need to taking special care. For specific rule of character escaping rule in property value, check https://docs.oracle.com/javase/7/docs/api/javax/management/ObjectName.html
This reverts commit fc2265d.
I have updated the code example at issue #15 (comment). The new code example demonstrates the new way to operate with the immutable design There are still two tasks to do
|
等你有空把測試補完這隻PR才能審視or合併
這可以作為另一隻PR 可以先花點時間看書看點資料,然後再回頭完善這隻PR (尤其是測試) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot 感謝更新,一樣繼續討論一下design的問題~
public BeanObject( | ||
String domainName, Map<String, String> properties, Map<String, Object> attributes) { | ||
this.domainName = domainName; | ||
this.properties = new HashMap<>(properties); // making defensive copy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果該map在內部也不打算修改的話,可以在建構時就先包成immutable object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
包成 immutable object 是指用Collections.unmodifiableMap()
嗎,雖然這個方法能否稱上 immutable 還有點爭議(曾經看過有人在 Reddit 上爭論這點, Java 的 unmodifiableMap 沒辦法在 compile time 避免修改的程式碼出現), 總之這樣做不錯。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java 的 unmodifiableMap 沒辦法在 compile time 避免修改的程式碼
沒辦法,因為那是runtime時檢查。java目前缺乏類似的語法支援
if (this == o) return true; | ||
if (o == null || getClass() != o.getClass()) return false; | ||
BeanObject that = (BeanObject) o; | ||
return domainName.equals(that.domainName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
這邊看來是假設所有物件都不會是null,那麼在建構時應該也要檢查null
this.attributes = new HashMap<>(); | ||
} | ||
|
||
public BeanObject selectProperty(String key, String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectProperty
的用途是什麼?可以補上javadocs嗎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selectProperty()
用來標示 MBean 的 property,MBeanClient
後續會這些 properties 來索取對應的 Mbean。
可以補上javadocs嗎
ok
|
||
public BeanObject fetchAttribute(String attributeName) { | ||
HashMap<String, Object> attributesCopy = new HashMap<>(attributes); | ||
attributesCopy.put(attributeName, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一般是不建議在map裡面放null,可以說明一下用途嗎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
這部分就有點尷尬了,當初換成 immutable 後很多原先還能理解的情況都變了。
原先的設計中(非 immutable 的版本), BeanObject
同一個物件實例可以被用來重複更新 attributes (BeanObject::updateAttributeValue
)。這個時候我打算用 null
來代表還沒被更新的情況。
後來換成 immutable 後, BeanObject
可能處於這兩個狀態
- 還沒被
MBeanClient
更新的BeanObjcet
. (attribute 欄位是 null) - 已經被
MBeanClient
更新的BeanObject
. (attribute 欄位非 null)
看起來 BeanObject
這個物件現在同時扮演了這兩個角色,所以才會有這種奇怪的情況發生。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
看起來 BeanObject 這個物件現在同時扮演了這兩個角色,所以才會有這種奇怪的情況發生。
同樣的故事。請問我們能否在建立BeanObject
時就確定它已經拿到properties and attributes。前面我們討論過每個BeanObject
都是"snapshot",換言之,每個BeanObject
從頭到腳都應該是immutable,使用者也不會試著去更新任何一個BeanObject
,例如:
List<BeanObject> beans = beanClient.query("your query);
// 接著把那些beans處理一下
if (!isClientConnected) throw new IllegalStateException("client is not connected yet"); | ||
} | ||
|
||
public void connect() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可否避免使用者建構完物件還需要在connect
? 這樣的做法會造成使用者手上的MBeanClient
有三種狀態,一種是還沒呼叫connect
、一種是有呼叫connect
但可能有錯誤、最後一種才是有呼叫connect
且成功。這樣會造成使用者的困擾。
反之,如果在建構時就建立連線,那麼使用者拿到MBeanClient
時就可以很肯定手上的物件一定是可以連線的狀態
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感謝指正。
當初會這樣做是在某個地方聽了 "別在 constructor 內丟 exception" 的說法,不過我剛剛 google 後似乎是我記錯了, 原先的 connect()
寫法導致的狀態問題也可能使 try-with-resource 等寫法造成麻煩,改成在 constructor 連線沒問題。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot 能否先請你補上測試?因為有些APIs的設計我有點疑惑,但因為你現在還沒補上任何用法,因此就有點難理解你設計的用意。另外你開始寫測試也能自己重新審視一下你設計的APIs好不好用
謝謝學長,我會開始補上這些測試。 |
client.queryObject(BeanQuery.forDomainName("org.example").whereProperty("type", "object1")); | ||
BeanObject queryResult2 = query.stream().findFirst().get(); | ||
// act 2 update fetch target's attribute | ||
BeanObject fetchTarget = queryResult2.fetchAttribute("Value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前看到一個比較大的問題是 query 到的 MBean, 需要手動去指定要索取的 attribute 名稱。
如果能夠用 MBeanServerConnection#getMBeanInfo 自動索取所有 attribute 的話可能會方便一些
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果能夠用 MBeanServerConnection#getMBeanInfo 自動索取所有 attribute 的話可能會方便一些
對,你說的沒錯,可否考慮用用看?透過該方法就可以建立BeanObject
with all properties and attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
概念上就是先呼叫getMBeanInfo
拿到attribute names,然後再呼叫getAttributes去拿到attribute value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot 感謝更新patch
感覺上似乎這隻patch想要把各種可能的用法都列出來,但我們接下來的任務真的需要這麼多用法嗎?例如BeanObject queryBean(BeanQuery beanQuery, String[] attributeNameList)
,我們接下來的任務會用到嗎?
豐富的APIs是件好事,但如果只是像遙控器介面那樣把各式各樣功能攤開,而不是根據實際狀況去設計,那就有點畫蛇添足,而且又會導致各個APIs的一致性不佳(例如為何有回傳一個BeanObject
和多個BeanObject
但又吃相同參數的狀況...)
我這邊會建議一開始可以稍微收斂一下,只設計會用且該用的APIs(也就是寫對的程式碼)
public BeanObject( | ||
String domainName, Map<String, String> properties, Map<String, Object> attributes) { | ||
this.domainName = Objects.requireNonNull(domainName); | ||
this.properties = Map.copyOf(Objects.requireNonNull(properties)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我倒是覺得兩個map都應該先請洗一下資料,把帶有null key and null value都拿掉。這樣對使用的人而言會比較安全
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
原來還有這種方法,感謝提醒
.filter(str -> !attributes.containsKey(str)) | ||
.collect(Collectors.toSet()); | ||
for (String attributeName : notResolvedAttributes) { | ||
attributes.put(attributeName, fetchAttributeObjectOrException(beanQuery, attributeName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
為何這裡要再嘗試拿一次attribute? 前面不是已經呼叫過一次了嗎?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
前面是用 MBeanServerConnection#getAttributes 取過資料了沒錯,不過那個 API 的 Javadoc 有說
If one or more attributes cannot be retrieved for some reason, they will be omitted from the returned AttributeList. The caller should check that the list is the same size as the attributes array. To discover what problem prevented a given attribute from being retrieved, call getAttribute for that attribute.
簡單來說如果 MBeanServerConnection#getAttributes
索取 Mbean 的過程中,如果因為意外不能索取這些 attribute, 他們會直接從回傳的結果中被忽略那些 attributes。如果要知道實際上發生了什麼問題,需要呼叫 MBeanServerConnection#getAttribute
來得知實際原因。
為了避免 library user 的困擾,所以我在程式碼裡面幫他們把這部分給做了。
* @return A {@link BeanObject} contain all attributes if target resolved successfully. | ||
* @throws InstanceNotFoundException If the pattern target doesn't exists on remote mbean server. | ||
*/ | ||
public BeanObject queryBean(BeanQuery beanQuery) throws InstanceNotFoundException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
使用者要如何知道是一個BeanObject
還是多個BeanObject
? 如果使用者以為是一個但實際上是多個,那為何不需要處理錯誤?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
使用者要如何知道是一個BeanObject還是多個BeanObject?
BeanQuery
內有用到 Pattern ("*“, "?"), 那回來的結果就有可能是多個。
如果使用者以為是一個但實際上是多個,那為何不需要處理錯誤?
基本上如果他們對 MBeanClient#queryBean()
用 Pattern 版本的 BeanQuery
,那麼他會得到 InstanceNotFoundException: "java.lang:type=Memo?y"
之類的錯誤
那個函數其實是寫給 仔細觀察最後 return 的部分,有呼叫到
如果那個用法真的礙眼的話我們可以把它 private 修飾。
這部分就很尷尬了,他們之間的差異在接受的
要用形態去區分這二者的差異還蠻困難的,因為是不是 Pattern 全看傳入的字串是否包含 "*" 或 "?",我對 Java 的瞭解有限,不知道能不能在 Compile Time 阻擋這種差異。我後來想不到方法所以就決定把這些細節寫在 javadoc 然後相信使用者了。 如果只釋出
謝謝學長,下次會注意的 |
這個方法有機會用到嗎?只索取一個mbean。可以試著從kafka metrics來思考,有哪個情境下會只要一個mbean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot 非常感謝貢獻,我覺得只差一點就可以合併了,再麻煩看一下,謝謝
mBeanServerConnection.queryMBeans(beanQuery.objectName(), null); | ||
|
||
// transform result into a set of BeanQuery | ||
Stream<BeanQuery> queries = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不太確定為何這裡要把ObjectName
拆解回BeanQuery
,然後再用BeanQuery
去做查詢 (tryQueryBean
),可是在實作上,BeanQuery
內部也是保存ObjectName
,這樣感覺有點多此一舉...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
當初這樣做是單純為了實作上的方便。
BeanQuery
是 ObjectName
比較現代化的 Wrapper。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果queryBean不開放的話,輸入參數應該考慮用ObjectName,這樣不用無謂的轉換。wrapper 用在開放的apis 上很好,可以讓其更好用,但如過是在物件內部的互動,那這個例子就比較畫蛇添足了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我很難用言語形容,但我有一種預感,移除那個轉換使用 ObjectName
,會給其他情境帶來很多副作用
現在的形態轉變是這樣 Set<ObjectName>
-> Set<BeanQuery>
-> Set<BeanObject>
看起來我們希望是 Set<ObjectName>
-> Set<BeanObject>
為了達成下面那種形態轉變,我們要付出的代價是 BeanQuery
到 BeanObject
那段程式碼不能重用。
或許我們能把程式碼弄成 Set<BeanQuery>
-> Set<ObjectName>
-> Set<BeanObject>
,但這樣做會遇到 ObjectName
老舊設計所帶來的麻煩,如果我們嘗試直接從 ObjectName
去生成 BeanObject
, 觀察 BeanObject
的 constructor, 他需要
String domainName
Map<String, String> properties
Map<String, Object> attributes
真正的麻煩在 properties
, 我們可以從 ObjectName#getKeyPropertyList
取得 properties
, 但是他的形態是 Hashtable
, 我們必須要重新複製整個 Hashtable
成為 HashMap
,如果我們可以從 BeanQuery
到 BeanObject
(現在的做法),那麼可以透過 Map.copyOf()
的優化避免掉這個 Map 的過度複製。
另外一個解決方案就是把 BeanQuery
的責任抽象化,類似我在 f816396 這個已經被 revert 的 commit 做的事情。在這個情境下只有 Set<Query>
-> Set<BeanObject>
,底下 Query
的實作為何已經被抽象走了,裡面可能是一個 BeanQuery
或 ObjectNameWrapper
實作。
總結我的看法:
- 把那個轉換改掉 (獨立的
Set<ObjectName>
->Set<BeanObject>
程式碼)- 程式碼不能重用
- 把那個轉變改掉 (
Set<BeanQuery>
->Set<ObjectName>
->Set<BeanObject>
)- 會複製很多
HashMap
- 會複製很多
- 維持現在的方案 (
Set<ObjectName>
->Set<BeanQuery>
->Set<BeanObject>
)- 程式碼可以重用
- 至少
BeanQuery
到BeanObject
這段程式碼不會複製HashMap
- 使用 Interface 抽象
BeanQuery
- 可行,但這樣程式碼好像會便得複雜(多一層抽象
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok 沒關係 這邊先這樣,可以之後再處理
.map(BeanQuery::fromObjectName); | ||
|
||
// execute query on each BeanQuery, return result as a set of BeanObject | ||
Set<BeanObject> queryResult = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
這裡用Set
而不是用List
有啥用意嗎?BeanObject
有可能重複?但BeanObject
也沒實作equals
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
當初是受 MBeanServerConnection#queryMBeans 回傳 Set
影響而做這個決定,單傳強調回傳的結果沒有順序關係。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果你真的想要用Set的話,要考慮實作equals, hashCode,不然就只是仰賴memory address,這樣排序沒有意義。例如ObjectName就有實作,用來代表不會有重複的
另外代表無序可以用Collection or Iterable等等,這樣意義比較明確
void setUp() throws IOException { | ||
JMXServiceURL serviceURL = new JMXServiceURL("service:jmx:rmi://127.0.0.1"); | ||
|
||
mBeanServer = ManagementFactory.getPlatformMBeanServer(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可否自己註冊一些mbeans然後試著用你設計的APIs把它撈出來?
當初 #10 裡面有一個需求是取得記憶體使用狀況,這個記錄在 |
所以之後實作#10的時候,你打算要一次一個mbean撈,還是一次撈多一點(例如用wildcard)一次撈回多個mbeans? |
看情況,如果是撈 Memory 應該是一次一撈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot thanks for updating patch. a couple of comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyparrot 我覺得這隻patch已經相當不錯了,晚點我會再看一次然後合併。
感謝你的努力以及耐心!
return this; | ||
} | ||
|
||
public BeanQueryBuilder noPropertyListPattern() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usePropertyListPattern
預設是false
,這樣的話這隻API就有點多餘。或許我們應該刪掉它,然後在usePropertyListPattern
補充一些說明預設是關閉,以及啟動後可以有哪些效果
Context: This PR is related to task 1 in #10
This PR implements two things.
BeanObject
, represent an MBean at the remote MBean server.MBeanClient
, the client we used to fetch & query MBean from an MBean server.Code Example