StyleTang commented on a change in pull request #17: URL: https://github.com/apache/rocketmq-dashboard/pull/17#discussion_r702443299
########## File path: src/main/java/org/apache/rocketmq/dashboard/admin/MQAdminFactory.java ########## @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.admin; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.acl.common.AclClientRPCHook; +import org.apache.rocketmq.acl.common.SessionCredentials; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.remoting.RPCHook; +import org.apache.rocketmq.tools.admin.DefaultMQAdminExt; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MQAdminFactory { + private Logger logger = LoggerFactory.getLogger(this.getClass()); + private RMQConfigure rmqConfigure; + + public MQAdminFactory(RMQConfigure rmqConfigure) { + this.rmqConfigure = rmqConfigure; + } + + public MQAdminExt getInstance() throws Exception { + DefaultMQAdminExt mqAdminExt = null; + RPCHook rpcHook = null; + boolean isEnableAcl = !StringUtils.isEmpty(rmqConfigure.getAccessKey()) + && !StringUtils.isEmpty(rmqConfigure.getSecretKey()); + if (isEnableAcl) { + SessionCredentials credentials = new SessionCredentials(rmqConfigure.getAccessKey(), + rmqConfigure.getSecretKey()); + rpcHook = new AclClientRPCHook(credentials); + } + mqAdminExt = new DefaultMQAdminExt(rpcHook); + mqAdminExt.setVipChannelEnabled(false); Review comment: Why it is false. It seems it should use system property ``` com.rocketmq.sendMessageWithVIPChannel ``` ########## File path: src/main/java/org/apache/rocketmq/dashboard/admin/MQAdminPooledObjectFactory.java ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.admin; + +import org.apache.commons.pool2.PooledObject; +import org.apache.commons.pool2.PooledObjectFactory; +import org.apache.commons.pool2.impl.DefaultPooledObject; +import org.apache.rocketmq.common.protocol.body.ClusterInfo; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MQAdminPooledObjectFactory implements PooledObjectFactory<MQAdminExt> { + + private final Logger logger = LoggerFactory.getLogger(this.getClass()); Review comment: ```java @Slf4j ``` ########## File path: src/main/java/org/apache/rocketmq/dashboard/aspect/admin/MQAdminAspect.java ########## @@ -57,19 +54,11 @@ public Object aroundMQAdminMethod(ProceedingJoinPoint joinPoint) throws Throwabl long start = System.currentTimeMillis(); Object obj = null; try { - MethodSignature signature = (MethodSignature)joinPoint.getSignature(); - Method method = signature.getMethod(); - MultiMQAdminCmdMethod multiMQAdminCmdMethod = method.getAnnotation(MultiMQAdminCmdMethod.class); - if (multiMQAdminCmdMethod != null && multiMQAdminCmdMethod.timeoutMillis() > 0) { - MQAdminInstance.initMQAdminInstance(multiMQAdminCmdMethod.timeoutMillis(),rmqConfigure.getAccessKey(),rmqConfigure.getSecretKey(), rmqConfigure.isUseTLS()); - } - else { - MQAdminInstance.initMQAdminInstance(0,rmqConfigure.getAccessKey(),rmqConfigure.getSecretKey(), rmqConfigure.isUseTLS()); - } + MQAdminInstance.createMQAdmin(mqAdminExtPool); Review comment: in case of race condition, MQAdminExt mqAdminExt can not be a static value, may be we should new MQAdminInstance here. ########## File path: src/main/java/org/apache/rocketmq/dashboard/service/client/MQAdminInstance.java ########## @@ -16,29 +16,23 @@ */ package org.apache.rocketmq.dashboard.service.client; -import org.apache.commons.lang3.StringUtils; -import org.apache.rocketmq.acl.common.AclClientRPCHook; -import org.apache.rocketmq.acl.common.SessionCredentials; -import org.apache.rocketmq.client.exception.MQClientException; +import org.apache.commons.pool2.impl.GenericObjectPool; import org.apache.rocketmq.client.impl.MQClientAPIImpl; import org.apache.rocketmq.client.impl.factory.MQClientInstance; -import org.apache.rocketmq.remoting.RPCHook; import org.apache.rocketmq.remoting.RemotingClient; -import org.apache.rocketmq.tools.admin.DefaultMQAdminExt; import org.apache.rocketmq.tools.admin.DefaultMQAdminExtImpl; import org.apache.rocketmq.tools.admin.MQAdminExt; import org.joor.Reflect; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class MQAdminInstance { - private static final ThreadLocal<DefaultMQAdminExt> MQ_ADMIN_EXT_THREAD_LOCAL = new ThreadLocal<DefaultMQAdminExt>(); - private static final ThreadLocal<Integer> INIT_COUNTER = new ThreadLocal<Integer>(); + + private static final Logger LOGGER = LoggerFactory.getLogger(MQAdminInstance.class); + private static MQAdminExt mqAdminExt; Review comment: static means this is a singleton object, but multiple threads will assign a value to this object, there will be concurrency problems here. ########## File path: src/main/java/org/apache/rocketmq/dashboard/admin/MQAdminFactory.java ########## @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.admin; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.acl.common.AclClientRPCHook; +import org.apache.rocketmq.acl.common.SessionCredentials; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.remoting.RPCHook; +import org.apache.rocketmq.tools.admin.DefaultMQAdminExt; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MQAdminFactory { + private Logger logger = LoggerFactory.getLogger(this.getClass()); + private RMQConfigure rmqConfigure; + + public MQAdminFactory(RMQConfigure rmqConfigure) { + this.rmqConfigure = rmqConfigure; + } + + public MQAdminExt getInstance() throws Exception { + DefaultMQAdminExt mqAdminExt = null; + RPCHook rpcHook = null; + boolean isEnableAcl = !StringUtils.isEmpty(rmqConfigure.getAccessKey()) + && !StringUtils.isEmpty(rmqConfigure.getSecretKey()); + if (isEnableAcl) { + SessionCredentials credentials = new SessionCredentials(rmqConfigure.getAccessKey(), + rmqConfigure.getSecretKey()); + rpcHook = new AclClientRPCHook(credentials); + } + mqAdminExt = new DefaultMQAdminExt(rpcHook); Review comment: 1. VipChannelEnabled feature is missing [ISSUE #773] add useTLS configure for rocketmq console 2. Use pool we can not set timeout for every single request, it will use default timeout 5000 ms. ########## File path: src/main/java/org/apache/rocketmq/dashboard/config/PoolConfig.java ########## @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.config; + +import org.apache.commons.pool2.impl.GenericObjectPool; +import org.apache.commons.pool2.impl.GenericObjectPoolConfig; +import org.apache.rocketmq.dashboard.admin.MQAdminFactory; +import org.apache.rocketmq.dashboard.admin.MQAdminPooledObjectFactory; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +@Configuration +public class PoolConfig { Review comment: It seems this Class is not a config but a object pool ########## File path: src/main/java/org/apache/rocketmq/dashboard/config/PoolConfig.java ########## @@ -0,0 +1,49 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.config; + +import org.apache.commons.pool2.impl.GenericObjectPool; +import org.apache.commons.pool2.impl.GenericObjectPoolConfig; +import org.apache.rocketmq.dashboard.admin.MQAdminFactory; +import org.apache.rocketmq.dashboard.admin.MQAdminPooledObjectFactory; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; + +@Configuration +public class PoolConfig { + + @Autowired + private RMQConfigure rmqConfigure; + + @Bean + public GenericObjectPool<MQAdminExt> mqAdminExtPool() { + GenericObjectPoolConfig genericObjectPoolConfig = new GenericObjectPoolConfig(); + genericObjectPoolConfig.setTestWhileIdle(true); + genericObjectPoolConfig.setMaxTotal(3); Review comment: Is 3 enough? Could you make it configurable? ########## File path: src/main/java/org/apache/rocketmq/dashboard/admin/MQAdminFactory.java ########## @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.rocketmq.dashboard.admin; + +import org.apache.commons.lang3.StringUtils; +import org.apache.rocketmq.acl.common.AclClientRPCHook; +import org.apache.rocketmq.acl.common.SessionCredentials; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.remoting.RPCHook; +import org.apache.rocketmq.tools.admin.DefaultMQAdminExt; +import org.apache.rocketmq.tools.admin.MQAdminExt; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class MQAdminFactory { + private Logger logger = LoggerFactory.getLogger(this.getClass()); Review comment: Can use ```java @Slf4j public class MQAdminFactory { ... } ``` instead of ```java private Logger logger = LoggerFactory.getLogger(this.getClass()); ``` ########## File path: src/main/java/org/apache/rocketmq/dashboard/service/impl/OpsServiceImpl.java ########## @@ -49,6 +55,8 @@ @Override public void updateNameSvrAddrList(String nameSvrAddrList) { configure.setNamesrvAddr(nameSvrAddrList); + // when update namesrvAddr, clean the mqAdminExt objects pool. + mqAdminExtPool.clear(); Review comment: Good for clear the pool~ Beside this, there is also LTS and VIPChannel config etc. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
