You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/04/27 01:14:55 UTC
[GitHub] [helix] rabashizade opened a new pull request #1715: Change MessageQueueMonitor from static to dynamic metric
rabashizade opened a new pull request #1715:
URL: https://github.com/apache/helix/pull/1715
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1683
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This PR modifies the MessageQueueMonitor class to use dynamic metrics by extending the DynamicMBeanProvider class. It also deprecates the MessageQueueMonitorMBean interface because it is no longer used.
### Tests
- [x] The following tests are written for this issue:
- The following is the result of the "mvn test" command on the appropriate module:
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestExpandCluster.testClusterExpansion:61 expected:<false> but was:<true>
[ERROR] TestDropCurrentStateRunningTask.testJobCurrentStateDroppedAfterCompletion:190 ยป Helix
[INFO]
[ERROR] Tests run: 1265, Failures: 2, Errors: 0, Skipped: 2
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:47 h
[INFO] Finished at: 2021-04-26T17:44:23-07:00
[INFO] ------------------------------------------------------------------------
```
TestExpandCluster:
```
Shut down zookeeper at port 2183 in thread main
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 93.181 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
```
TestDropCurrentStateRunningTask:
```
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 80.98 s - in org.apache.helix.integration.task.TestDropCurrentStateRunningTask
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
```
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
rabashizade commented on pull request #1715:
URL: https://github.com/apache/helix/pull/1715#issuecomment-828623162
This PR is ready to be merged, approved by @jiajunwang.
**Commit message:**
Change MessageQueueMonitor from static to dynamic metric
This commit modifies the MessageQueueMonitor class to use dynamic metrics by extending the DynamicMBeanProvider class. Since after this change the MessageQueueMonitorMBean interface is no longer used in the code, this interface is deleted to remove all direct references to the MBean class.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1715:
URL: https://github.com/apache/helix/pull/1715#discussion_r621502160
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/MessageQueueMonitor.java
##########
@@ -70,9 +72,9 @@ public void init() {
* Remove this bean from the server
*/
public void reset() {
- _messageQueueBacklog = 0;
+ _messageQueueBacklog.updateValue(0L);
try {
- unregister(getObjectName(getBeanName()));
+ unregister();
} catch (Exception e) {
LOG.error("Fail to register MessageQueueMonitor", e);
Review comment:
It's defined in the parent class (DynamicMBeanProvider).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1715:
URL: https://github.com/apache/helix/pull/1715#discussion_r620807951
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/MessageQueueMonitorMBean.java
##########
@@ -21,6 +21,7 @@
import org.apache.helix.monitoring.SensorNameProvider;
+@Deprecated
Review comment:
Please remove it. There should be no direct reference to the MBean class. All usage should be done through the MBeanServer, so it is automatically backward compatible, and this class shall be removed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1715:
URL: https://github.com/apache/helix/pull/1715#discussion_r621481848
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/MessageQueueMonitor.java
##########
@@ -70,9 +72,9 @@ public void init() {
* Remove this bean from the server
*/
public void reset() {
- _messageQueueBacklog = 0;
+ _messageQueueBacklog.updateValue(0L);
try {
- unregister(getObjectName(getBeanName()));
+ unregister();
} catch (Exception e) {
LOG.error("Fail to register MessageQueueMonitor", e);
Review comment:
NIT: Am I missed something in another PR? I did not find the unregister() function.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a change in pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1715:
URL: https://github.com/apache/helix/pull/1715#discussion_r621425730
##########
File path: helix-core/src/main/java/org/apache/helix/monitoring/mbeans/MessageQueueMonitorMBean.java
##########
@@ -21,6 +21,7 @@
import org.apache.helix.monitoring.SensorNameProvider;
+@Deprecated
Review comment:
Thanks for the comment, I deleted the interface.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc merged pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #1715:
URL: https://github.com/apache/helix/pull/1715
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on pull request #1715: Change MessageQueueMonitor from static to dynamic metric
Posted by GitBox <gi...@apache.org>.
rabashizade commented on pull request #1715:
URL: https://github.com/apache/helix/pull/1715#issuecomment-827773745
> LGTM. Could you please double check where the monitor gets used, including in the test, and make sure there won't be any regression.
Thanks for the comment. I double-checked, the monitor is only used in the HelixTaskExecutor class, and is only instantiated in 3 other places (MockHelixTaskExecutor, TestP2PNoDuplicatedMessage, and DefaultMessagingService) none of which resulted in any issues with changing the metric.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org