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

[ISSUE #598] Enhance transaction by putting messages that exceed max check times to system topic #633

Merged
merged 7 commits into from Jun 19, 2019

Conversation

@xiangwangcheng
Copy link
Contributor

xiangwangcheng commented Dec 25, 2018

What is the purpose of the change

Fix #598

Brief changelog

For half messages that check times exceed the max check times(15 by default), not doing anything in here

    @Override
    public void resolveDiscardMsg(MessageExt msgExt) {
        log.error("MsgExt:{} has been checked too many times, so discard it", msgExt);
    }

and instead of this, put it into a system topic will be more meaningful. And you can also reopen message check logic manually.
(The logic of reopen message check will be submitted later.)

Brief Design:

  1. Complement the logic of DefaultTransactionalMessageCheckListener#resolveDiscardMsg to put half messages to a system topic TRANS_CHECK_MAX_TIME_TOPIC.
  2. add a new RequestCode RESUME_CHECK_HALF_MESSAGE(323) to enable resuming check logic by using DefaultMQAdminExt

Modification:

  1. Implement DefaultTransactionalMessageCheckListener#resolveDiscardMsg
  2. add resumeCheckHalfMessage method to AdminBrokerProcessor.java to recieve resume request from client.
  3. add resumeCheckHalfMessage in MQClientAPIImpl.java to send resume request to broker.

Verifying this change

XXXX

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

  • [√] Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • [√] Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • [√] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [√] Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • [√] Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 25, 2018

Coverage Status

Coverage increased (+0.1%) to 50.479% when pulling 8f973de on xiangwangcheng:enhance_transaction into bf2a7e5 on apache:develop.

@duhenglucky

This comment has been minimized.

Copy link
Contributor

duhenglucky commented Jan 4, 2019

@xiangwangcheng please rebase first and make sure this PR only contains your commit history.

@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch from 8cf0d6a to 5034136 Jan 4, 2019
@xiangwangcheng

This comment has been minimized.

Copy link
Contributor Author

xiangwangcheng commented Jan 4, 2019

@duhenglucky I already did rebase and now the commit history is much better.

@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch from 36aff5b to c8f1fe3 Jan 14, 2019
@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch from c8f1fe3 to 8b56256 Feb 17, 2019
TopicConfig topicConfig = this.getBrokerController().getTopicConfigManager()
.createTopicOfTranCheckMaxTime(TCMT_QUEUE_NUMS,
PermName.PERM_READ | PermName.PERM_WRITE);
int queueId = Math.abs(random.nextInt() % 99999999) % TCMT_QUEUE_NUMS;

This comment has been minimized.

Copy link
@zongtanghu

zongtanghu Mar 7, 2019

Contributor

The TRANS_CHECK_MAX_TIME_TOPIC's queue num is 1,so the variable queueId should be set to 1,directly.

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

Yes, as @zongtanghu comment, it has the difference with sendBackMessage method, so if only one queue in this inner topic, please use a constant instead.

@zongtanghu zongtanghu added this to the 4.5.0 milestone Mar 7, 2019
Copy link
Contributor

duhenglucky left a comment

Thanks for @xiangwangcheng's excellent PR, and we have put forward some comments for reference.


import org.apache.rocketmq.remoting.protocol.RemotingSerializable;

public class ResumeCheckHalfMessageResult extends RemotingSerializable {

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

Put this part of the content into the header and it seems that we don't need an API return some metrics data in response.


package org.apache.rocketmq.common.protocol.body;

public enum ResumeResult {

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

IMO, it would be nice if we can use the ResponseCode directly.

@@ -454,7 +454,19 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.6.3</version>
<version>2.23.0</version>

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

Why need to import other mockito packages here?


if (requestHeader.getTopic().equals(this.brokerController.getBrokerConfig().getBrokerClusterName())) {
String errorMsg = "the topic[" + requestHeader.getTopic() + "] is conflict with system reserved words.";
(CreateTopicRequestHeader) request

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

If no other changes, please don't reformat the code :)


public DefaultTransactionalMessageCheckListener() {
super();
}

@Override
public void resolveDiscardMsg(MessageExt msgExt) {
log.error("MsgExt:{} has been checked too many times, so discard it", msgExt);
log.error(
"MsgExt:{} has been checked too many times, so discard it by moving it to system topic TRANS_CHECK_MAXTIME_TOPIC",

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

I wonder if we can change the log format to "[NOTIFYME]MsgExt: ...."?

This comment has been minimized.

Copy link
@ahuazhu
TopicConfig topicConfig = this.getBrokerController().getTopicConfigManager()
.createTopicOfTranCheckMaxTime(TCMT_QUEUE_NUMS,
PermName.PERM_READ | PermName.PERM_WRITE);
int queueId = Math.abs(random.nextInt() % 99999999) % TCMT_QUEUE_NUMS;

This comment has been minimized.

Copy link
@duhenglucky

duhenglucky Mar 11, 2019

Contributor

Yes, as @zongtanghu comment, it has the difference with sendBackMessage method, so if only one queue in this inner topic, please use a constant instead.

@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch 3 times, most recently from 780b2bb to 76d700e Mar 11, 2019
@ShannonDing ShannonDing modified the milestones: 4.5.0, 4.6.0 Apr 10, 2019
@zongtanghu zongtanghu modified the milestones: 4.6.0, 4.5.2 Jun 17, 2019
@zongtanghu

This comment has been minimized.

Copy link
Contributor

zongtanghu commented Jun 17, 2019

Firstly,please resolve the confict codes in this pr. @xiangwangcheng And then,please help to review this pr. @ShannonDing @jonnxu @wlliqipeng Thanks very much.

@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch from 71c1df1 to 729f33f Jun 17, 2019
Copy link
Member

ShannonDing left a comment

LGTM

@zongtanghu

This comment has been minimized.

Copy link
Contributor

zongtanghu commented Jun 18, 2019

LGTM

@jonnxu

This comment has been minimized.

Copy link

jonnxu commented Jun 18, 2019

good idea, LGTM

@xiangwangcheng xiangwangcheng force-pushed the xiangwangcheng:enhance_transaction branch from 729f33f to 8f973de Jun 18, 2019
@duhenglucky duhenglucky merged commit d66243c into apache:develop Jun 19, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 50.479%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.