Hi Shenglin One thing which you are doing well is introducing subresources, given that both MBean and MBeanCotellection have become the ones. Understanding how subresources work is good in itself. I can see you have unique ids allocated to MBeans now. So there's some progress. IMHO some more code cleanup is needed.
1. At this stage please have only a ***single*** subresource, intoroduce a dedicated class called MBeanResource which should have only a single member, MBean instance. This MBeanResource should only have right now setMBean(MBean) and getMBean methods. getMBean method should be annotated with @GET. 2. MBean and MBeanCollection should have no static methods, and no JAX-RS resource methods at all. These are plain JAXB beans only. 3. JMXServer should have: - /mbean/{id} Note, Path value should be '/mbean/{id}' You should use a captured {id}, such as 0, 1, 2, etc (as opposed to manually parsing path segments) for locating MBean, set this MBean on a new MBeanResource instance and return MBeanResource, MBeanResource.getMBean will complete the processing. we will think later on how to keep MBeanResources between requests. The reason I'd like a dedicated MBeanResource wrap MBean is because possible modifications, update notifications, etc may need to be dealt with and thus IMHO it can be cleaner to keep MBean as a plain JAXB bean and have a dedicated JMX-aware handler around it as opposed to making MBean both JAXB and JMX-aware at the same time. - /list - /{property}/{value} Please note this method, /{property}/{value}, is on JMXServer. Remove @Path("") and replace it with /{property}/{value}. - /objectname/{objectname} This method should iterate over the existing MBeanCollection only without quering the instrumentation manager again. 3. All the methods should use the same code for accessing the map - which is what you already do. Some cleanup is needed there too, but for now please address the above thanks, Sergey On Thu, Jun 9, 2011 at 4:05 AM, Shenglin Qiu <dabaip...@hotmail.com> wrote: > Hi Sergey: > > Sorry for replying a little bit late. > But I reverted the code as you mentioned /mbean/{id} must not be changed. > > Code has be synced to github: > https://github.com/dabaipang/services > > /** > * Specification: > * Function 1: > * Jmx Server: > * sub-resource locator > * http://localhost:8080/services/jmx/mbean/0 > * http://localhost:8080/services/jmx/mbean/1 > * ... > * > * > * Function 2: > * Note: search by bus.id, bus.id will be changed on every time server > had been bounced. > * http://localhost:8080/services/jmx/objectname/org.apache.cxf:* > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,* > * ... > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:port=%22CustomerServiceImpl%22,* > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,port=%22SoapPort%22,type=Bus.Service.Endpoint,* > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:type=Bus.Service.Endpoint,* > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,service=%22%7Bhttp%3A%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl%22,type=Bus.Service.Endpoint,* > * > http://localhost:8080/services/jmx/objectname/org.apache.cxf:service=%22%7Bhttp%3A%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl%22,* > * > * Function 3: > * Search by service: > * > http://localhost:8080/services/jmx/service/%7Bhttp:%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl > * > http://localhost:8080/services/jmx/service/%7Bhttp:%2F%2Fserver.gsoc.apache.org%2F%7DUserServiceImpl > * > * Note, above queries are searching these 2, but replace by url encoding > from: > * {http://server.gsoc.apache.org/}UserServiceImpl > * {http://server.gsoc.apache.org/}CustomerServiceImpl > * > * Search by type: > * http://localhost:8080/services/jmx/type/Bus.Service.Endpoint > * > * Search by port: > * http://localhost:8080/services/jmx/port/UserServiceImpl > * http://localhost:8080/services/jmx/port/CustomerServiceImpl > * http://localhost:8080/services/jmx/port/SoapPort > * > * Search by domain > * http://localhost:8080/services/jmx/domain/org.apache.cxf > * > * Function 4: > * http://localhost:8080/services/jmx/list > * > * */ > > Source is also attached. > Thank you very much on the reviewing, I am very dedicated to what you have > mentioned. Though there was a time in last 2 days, I reformatted the code a > little too much:) > > > Regards: > Shenglin Qiu > > >> Date: Wed, 8 Jun 2011 10:23:58 +0100 >> Subject: Re: Expose MBeans in CXF >> From: sberyoz...@gmail.com >> To: dabaip...@hotmail.com >> CC: dev@cxf.apache.org >> >> Hi Shenglin >> >> I'm not sure if I'm confusing you with the comments I'm making...The >> source has become cleaner, good progress there, >> but I'm not sure what are you doing there at all now. >> Why MBeanCollection has become a subresource ? What happened to our >> /mbean/{id} handler ? Recall, you had a subresource locator method >> dealing with '/mbean/{id}' and you were delegating to MBean to finish >> the processing and the only thing I suggested is to have a dedicated >> MBeanResource handler which I guess should wrap an individual MBean >> and work with it (get its state for now, etc) >> >> What does >> >> http://localhost:8080/services/jmx/property/id/0 >> >> mean ? >> >> it has to be >> >> http://localhost:8080/services/jmx/mbean/0 >> http://localhost:8080/services/jmx/mbean/1 >> >> and MBeanResource needs to deal with these URIs. >> >> Please check my previous email. >> have only >> 1. list all MBeans in org.apache.cxf domain >> /list >> 2. find MBeans in org.apache.cxf domain which have a given attribute value >> /{attribute}/{value} >> 3. find MBeans in org.apache.cxf domain their object names >> /objectname/{objectname} >> 4. get the represenation of the individual MBean (and later update it, >> register event listeners, etc) >> /mbean/{id} >> >> // we haven't discussed it yet - need to be done >> 5. Return the counter statistics >> >> /counters >> >> 6. Counters for a specific endpoint >> >> /counters/{servicename} >> >> Sergey >> >> On Wed, Jun 8, 2011 at 5:46 AM, Shenglin Qiu <dabaip...@hotmail.com> >> wrote: >> > Hi Sergey: >> > >> > I attached the src and due to I am working on the mvn project only sync >> > to >> > svn, so I will take some time and sync this to github. >> > >> > And here is an update on the request: >> > >> > Self-defined demo inbound service: >> > http://localhost:8080/services/greeter?wsdl >> > http://localhost:8080/services/customerservice/customers >> > http://localhost:8080/services/customerservice/customer/firstname/Lois >> > http://localhost:8080/services/customerservice/customer/id/1 >> > http://localhost:8080/services/userservice/users >> > http://localhost:8080/services/userservice/user/1 >> > /** >> > * Specification: >> > * Function 1: >> > * Jmx Server: >> > * sub-resource locator >> > * http://localhost:8080/services/jmx/property/id/0 >> > * http://localhost:8080/services/jmx/property/id/1 >> > * ... >> > * >> > * >> > * Function 2: >> > * Note: search by bus.id, bus.id will be changed on every time >> > server >> > had been bounced. >> > * http://localhost:8080/services/jmx/objectname/org.apache.cxf:* >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,* >> > * ... >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:port=%22CustomerServiceImpl%22,* >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,port=%22SoapPort%22,type=Bus.Service.Endpoint,* >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:type=Bus.Service.Endpoint,* >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,service=%22%7Bhttp%3A%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl%22,type=Bus.Service.Endpoint,* >> > * >> > >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:service=%22%7Bhttp%3A%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl%22,* >> > * >> > * Function 3: >> > * Search by service: >> > * >> > >> > http://localhost:8080/services/jmx/property/service/%7Bhttp:%2F%2Fserver.gsoc.apache.org%2F%7DCustomerServiceImpl >> > * >> > >> > http://localhost:8080/services/jmx/property/service/%7Bhttp:%2F%2Fserver.gsoc.apache.org%2F%7DUserServiceImpl >> > * >> > * Note, above queries are searching these 2, but replace by url >> > encoding >> > from: >> > * {http://server.gsoc.apache.org/}UserServiceImpl >> > * {http://server.gsoc.apache.org/}CustomerServiceImpl >> > * >> > * Search by type: >> > * >> > http://localhost:8080/services/jmx/property/type/Bus.Service.Endpoint >> > * >> > * Search by port: >> > * http://localhost:8080/services/jmx/property/port/UserServiceImpl >> > * >> > http://localhost:8080/services/jmx/property/port/CustomerServiceImpl >> > * http://localhost:8080/services/jmx/property/port/SoapPort >> > * >> > * Function 4: >> > * http://localhost:8080/services/jmx/list >> > * >> > * */ >> > >> > I will ping you tomorrow morning. >> > >> > Thank you. >> > Regards: >> > >> > Shenglin Qiu >> > >> > >> >> Date: Wed, 1 Jun 2011 15:32:30 +0100 >> >> Subject: Re: Expose MBeans in CXF >> >> From: sberyoz...@gmail.com >> >> To: dev@cxf.apache.org >> >> >> >> Hi Shenglin >> >> >> >> It's a step in the right direction, thanks, but JMXServer still needs >> >> to be cleaned up quite a bit. >> >> - as I said earlier you have 4 big functions basically duplicating >> >> each other. We can't have a method per every attribute a given MBean >> >> may have. Have a single function only, max two (one capturing all the >> >> attributes, another one - dedicated to objectname/{value}). >> >> - Introduce MBeanResource subresource instead of overloading MBean, it >> >> will be cleaner and easier to work with >> >> - as suggested earlier, update the domain MBean and/or MBeanAttribute >> >> such that you could drop a big chunk of code in JMXServer where >> >> individual attribute values are set, currently in getMbeanMap (if >> >> (attrName.equals("service") then/else)). That is because a number of >> >> attributes is basically unlimited, even though we have some well-known >> >> ones >> >> >> >> - getMBeanMap - this is so complex I can't understand what it does. I >> >> do understand it returns a thread-safe Map - but I'm finding it >> >> difficult to understand how the method achieves that. Please remove >> >> all those synchronized blocks and have ConcurrentHashMap. Have atomic >> >> integer counter, synchronized exlicitly (in a block) if you prefer, >> >> and try to get a simple and effective function implemented. Don't make >> >> the logic of that function dependent on a n >> >> I don't understand why you use "list" for creating MBean hrefs; >> >> >> >> - there's no need for having Mbean.id and MBean.href because >> >> MBean.href is identifying a given MBean uniquely and thus you could >> >> use that calculated href as a key. >> >> >> >> Thanks, Sergey >> >> >> >> >> >> On Tue, May 31, 2011 at 5:36 PM, Shenglin Qiu <dabaip...@hotmail.com> >> >> wrote: >> >> > >> >> > Hi Sergey: >> >> > >> >> > Project has been synced to Github: >> >> > >> >> > Browser: >> >> > https://github.com/dabaipang/services >> >> > >> >> > >> >> > Git address: >> >> > https://dabaip...@github.com/dabaipang/services.git >> >> > or >> >> > g...@github.com:dabaipang/services.git >> >> > >> >> > >> >> > Thank you. >> >> > >> >> > >> >> > Regards: >> >> > Shenglin Qiu >> >> > >> >> > >> >> > >> >> >> From: dabaip...@hotmail.com >> >> >> To: sberyoz...@gmail.com >> >> >> CC: dev@cxf.apache.org >> >> >> Subject: RE: Expose MBeans in CXF >> >> >> Date: Tue, 31 May 2011 11:23:38 -0400 >> >> >> >> >> >> >> >> >> Hi Sergey: >> >> >> >> >> >> > > Function 1: (0 - 5 is continuous, no gap) >> >> >> > > Jmx Server: >> >> >> > > sub-resource locator >> >> >> > > http://localhost:8080/services/jmx/mbean/0 >> >> >> > > http://localhost:8080/services/jmx/mbean/1 >> >> >> > >> >> >> > The id allocation (0, 1, etc) has to be thread safe >> >> >> >> >> >> Done with: >> >> >> private synchronized Map<String, MBean> getMBeansMap(){ >> >> >> .... >> >> >> } >> >> >> My reason not using synchronized block but using synchronized method >> >> >> is: >> >> >> this guarantees the order on mbeansMap's initialization, so when >> >> >> it's >> >> >> called by a lot of requests, the followings will have to wait, unit >> >> >> the >> >> >> first one finished, after that, following requests will see >> >> >> mbeansMap has >> >> >> been initialized, and simply return mbeansMap without initializing >> >> >> it again. >> >> >> >> >> >> >> >> >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:* >> >> >> > or >> >> >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf >> >> >> > >> >> >> > should also work >> >> >> > >> >> >> > > >> >> >> >> >> >> http://localhost:8080/services/jmx/objectname/org.apache.cxf:* >> >> >> is working. >> >> >> >> >> >> > > Due to the URL duplication with /mbean/{id}, I can't put >> >> >> > > {searchtype}/{query}, so I put things like: >> >> >> > >> >> >> > Personally I'd prefer to keep a URI as minimal as possible. >> >> >> > >> >> >> > /mbean/{id} >> >> >> > and >> >> >> > {searchtype}/{query} >> >> >> > >> >> >> > do not duplicate each other given that /mbean/{id} is more >> >> >> > specific >> >> >> > than {searchtype}/{query}. >> >> >> > Likewise /objectname/{objectname} is more specific than >> >> >> > {searchtype}/{query}, so consider dropping /search/ part, >> >> >> > we may have a dedicated /search handler later on. >> >> >> >> >> >> Done by dedicating each request per method: >> >> >> @GET >> >> >> @POST >> >> >> @Path("/list") >> >> >> public MBeanCollection list() >> >> >> >> >> >> @Path("/mbean/") >> >> >> public MBean locateMBean() >> >> >> >> >> >> >> >> >> @GET >> >> >> @POST >> >> >> @Path("/type/{query}") >> >> >> public MBeanCollection searchMBeansByType(@PathParam("query") >> >> >> String query) >> >> >> >> >> >> >> >> >> @GET >> >> >> @POST >> >> >> @Path("/port/{query}") >> >> >> public MBeanCollection searchMBeansByPort(@PathParam("query") >> >> >> String query) >> >> >> >> >> >> >> >> >> @GET >> >> >> @POST >> >> >> @Path("/service/{query}") >> >> >> public MBeanCollection searchMBeansByService(@PathParam("query") >> >> >> String query) >> >> >> >> >> >> >> >> >> @GET >> >> >> @POST >> >> >> @Path("/objectname/{objectname}") >> >> >> public MBeanCollection getComponent(@PathParam("objectname") >> >> >> String objectname) >> >> >> >> >> >> I remembered you mentioned to handle Exceptions once, currently I am >> >> >> still 'throws' -ing them out, I will make the try catch block. >> >> >> Should I also add a new object which display the error or simply >> >> >> return >> >> >> empty? >> >> >> >> >> >> >> >> >> Thank you very much. >> >> >> >> >> >> >> >> >> Regards: >> >> >> Shenglin Qiu >> >> >> >> >> >> >> >> >> > Date: Tue, 31 May 2011 10:35:36 +0100 >> >> >> > Subject: Re: Expose MBeans in CXF >> >> >> > From: sberyoz...@gmail.com >> >> >> > To: dev@cxf.apache.org >> >> >> > >> >> >> > Hi Shenglin >> >> >> > >> >> >> > Good progress, some comments below >> >> >> > > >> >> >> > > Function 1: (0 - 5 is continuous, no gap) >> >> >> > > Jmx Server: >> >> >> > > sub-resource locator >> >> >> > > http://localhost:8080/services/jmx/mbean/0 >> >> >> > > http://localhost:8080/services/jmx/mbean/1 >> >> >> > >> >> >> > The id allocation (0, 1, etc) has to be thread safe >> >> >> > >> >> >> > > >> >> >> > > >> >> >> > > Function 2: >> >> >> > > Note: search by bus.id, bus.id will be changed on every time >> >> >> > > server >> >> >> > > had been >> >> >> > > bounced. >> >> >> > > >> >> >> > > >> >> >> > > http://localhost:8080/services/jmx/objectname/org.apache.cxf:bus.id=cxf28619341,* >> >> >> > > ... >> >> >> > that is ok, a user does not have to specify them as you >> >> >> > demonstrated >> >> >> > with queries like >> >> >> > >> >> >> > > >> >> >> > > >> >> >> > > http://localhost:8080/services/jmx/objectname/org.apache.cxf:port=%22CustomerServiceImpl%22,* >> >> >> > I guess >> >> >> > >> >> >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf:* >> >> >> > or >> >> >> > http://localhost:8080/services/jmx/objectname/org.apache.cxf >> >> >> > >> >> >> > should also work >> >> >> > >> >> >> > > >> >> >> > > >> >> >> > > I am only now use 2 methods to fulfill: >> >> >> > > 1 method for service/{serivce}, port/{port}, type/{type} >> >> >> > > 1 method for objectname/{objectname} >> >> >> > >> >> >> > OK >> >> >> > >> >> >> > > >> >> >> > > Due to the URL duplication with /mbean/{id}, I can't put >> >> >> > > {searchtype}/{query}, so I put things like: >> >> >> > >> >> >> > Personally I'd prefer to keep a URI as minimal as possible. >> >> >> > >> >> >> > /mbean/{id} >> >> >> > and >> >> >> > {searchtype}/{query} >> >> >> > >> >> >> > do not duplicate each other given that /mbean/{id} is more >> >> >> > specific >> >> >> > than {searchtype}/{query}. >> >> >> > Likewise /objectname/{objectname} is more specific than >> >> >> > {searchtype}/{query}, so consider dropping /search/ part, >> >> >> > we may have a dedicated /search handler later on. >> >> >> > >> >> >> > The other thing I forgot to mention, please remove all those >> >> >> > System.out.println, ping me please if you need some help with >> >> >> > setting >> >> >> > up a remote debugging session >> >> >> > >> >> >> > Thanks, Sergey >> >> >> > >> >> >> > > >> >> >> > > Thank you so much! >> >> >> > > >> >> >> > > Regards: >> >> >> > > Shenglin Qiu >> >> >> > > >> >> >> >> >> > >> >> >> >> >> >> >> >> -- >> >> Sergey Beryozkin >> >> >> >> Application Integration Division of Talend >> >> http://sberyozkin.blogspot.com >> > >> >> >> >> -- >> Sergey Beryozkin >> >> Application Integration Division of Talend >> http://sberyozkin.blogspot.com > -- Sergey Beryozkin Application Integration Division of Talend http://sberyozkin.blogspot.com