Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1.0-incubating
    • Component/s: None
    • Labels:
      None

      Description

      Quality documentation is critically important to develop and maintain a project. The better the documentation is, the
      easier it will be for other participants to understand and respond properly.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user lizhanhui opened a pull request:

          https://github.com/apache/incubator-rocketmq/pull/80

          ROCKETMQ-114 Add javadoc to codebase

          Add javadoc to codebase

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/lizhanhui/incubator-rocketmq ROCKETMQ-114

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-rocketmq/pull/80.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #80


          commit ef09bcc13fb9fee71255f78568e5c59fd252064b
          Author: Zhanhui Li <lizhanhui@apache.org>
          Date: 2017-03-06T09:46:53Z

          Add doc

          commit 678ea43fa77ebe213ea4f4c76fc3a3693dd44d2f
          Author: Zhanhui Li <lizhanhui@apache.org>
          Date: 2017-03-10T10:26:01Z

          Add javadoc.

          commit 5662cd46a46d0e8c7dacb5ce4be3beecefca78f2
          Author: Li Zhanhui <lizhanhui@apache.org>
          Date: 2017-03-16T03:24:27Z

          Add javadoc to send message processor.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user lizhanhui opened a pull request: https://github.com/apache/incubator-rocketmq/pull/80 ROCKETMQ-114 Add javadoc to codebase Add javadoc to codebase You can merge this pull request into a Git repository by running: $ git pull https://github.com/lizhanhui/incubator-rocketmq ROCKETMQ-114 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-rocketmq/pull/80.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #80 commit ef09bcc13fb9fee71255f78568e5c59fd252064b Author: Zhanhui Li <lizhanhui@apache.org> Date: 2017-03-06T09:46:53Z Add doc commit 678ea43fa77ebe213ea4f4c76fc3a3693dd44d2f Author: Zhanhui Li <lizhanhui@apache.org> Date: 2017-03-10T10:26:01Z Add javadoc. commit 5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 Author: Li Zhanhui <lizhanhui@apache.org> Date: 2017-03-16T03:24:27Z Add javadoc to send message processor.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          [![Coverage Status](https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410)

          Coverage increased (+0.2%) to 31.057% when pulling *5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui:ROCKETMQ-114* into *6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 [! [Coverage Status] ( https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410 ) Coverage increased (+0.2%) to 31.057% when pulling * 5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui: ROCKETMQ-114 * into * 6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          [![Coverage Status](https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410)

          Coverage increased (+0.2%) to 31.057% when pulling *5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui:ROCKETMQ-114* into *6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 [! [Coverage Status] ( https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410 ) Coverage increased (+0.2%) to 31.057% when pulling * 5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui: ROCKETMQ-114 * into * 6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          [![Coverage Status](https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410)

          Coverage increased (+0.2%) to 31.057% when pulling *5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui:ROCKETMQ-114* into *6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 [! [Coverage Status] ( https://coveralls.io/builds/10619410/badge)](https://coveralls.io/builds/10619410 ) Coverage increased (+0.2%) to 31.057% when pulling * 5662cd46a46d0e8c7dacb5ce4be3beecefca78f2 on lizhanhui: ROCKETMQ-114 * into * 6b7d206f09b928ea58fb3a62413a053f008b8c1c on apache:develop *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on a diff in the pull request:

          https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733685

          — Diff: broker/src/main/java/org/apache/rocketmq/broker/BrokerStartup.java —
          @@ -95,52 +141,65 @@ public static BrokerController createBrokerController(String[] args) {
          final BrokerConfig brokerConfig = new BrokerConfig();
          final NettyServerConfig nettyServerConfig = new NettyServerConfig();
          final NettyClientConfig nettyClientConfig = new NettyClientConfig();

          • nettyServerConfig.setListenPort(10911);
            + nettyServerConfig.setListenPort(10911); // Set default broker listen port
            final MessageStoreConfig messageStoreConfig = new MessageStoreConfig();

          if (BrokerRole.SLAVE == messageStoreConfig.getBrokerRole())

          { + + // Use a conservative value for slave brokers so that consumer groups won't frequently change targeting + // brokers when they are consuming messages at the boundary between being probably resident in physical + // memory and swapped out. int ratio = messageStoreConfig.getAccessMessageInMemoryMaxRatio() - 10; messageStoreConfig.setAccessMessageInMemoryMaxRatio(ratio); }
          • if (commandLine.hasOption('p')) {
            + // FIXME: log should not be null
              • End diff –

          Creating a JIRA issue for this and having its number as a comment is a good practice.

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733685 — Diff: broker/src/main/java/org/apache/rocketmq/broker/BrokerStartup.java — @@ -95,52 +141,65 @@ public static BrokerController createBrokerController(String[] args) { final BrokerConfig brokerConfig = new BrokerConfig(); final NettyServerConfig nettyServerConfig = new NettyServerConfig(); final NettyClientConfig nettyClientConfig = new NettyClientConfig(); nettyServerConfig.setListenPort(10911); + nettyServerConfig.setListenPort(10911); // Set default broker listen port final MessageStoreConfig messageStoreConfig = new MessageStoreConfig(); if (BrokerRole.SLAVE == messageStoreConfig.getBrokerRole()) { + + // Use a conservative value for slave brokers so that consumer groups won't frequently change targeting + // brokers when they are consuming messages at the boundary between being probably resident in physical + // memory and swapped out. int ratio = messageStoreConfig.getAccessMessageInMemoryMaxRatio() - 10; messageStoreConfig.setAccessMessageInMemoryMaxRatio(ratio); } if (commandLine.hasOption('p')) { + // FIXME: log should not be null End diff – Creating a JIRA issue for this and having its number as a comment is a good practice.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on a diff in the pull request:

          https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733797

          — Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/SendMessageProcessor.java —
          @@ -112,18 +156,21 @@ private RemotingCommand consumerSendMsgBack(final ChannelHandlerContext ctx, fin
          return response;
          }

          + // Make sure message store of the broker is writable.
          if (!PermName.isWriteable(this.brokerController.getBrokerConfig().getBrokerPermission()))

          { response.setCode(ResponseCode.NO_PERMISSION); response.setRemark("the broker[" + this.brokerController.getBrokerConfig().getBrokerIP1() + "] sending message is forbidden"); return response; }

          + // TODO: we should warn client here because OP may carelessly get things wrong here.
          — End diff –

          I think this needs another JIRA issue too.

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on a diff in the pull request: https://github.com/apache/incubator-rocketmq/pull/80#discussion_r114733797 — Diff: broker/src/main/java/org/apache/rocketmq/broker/processor/SendMessageProcessor.java — @@ -112,18 +156,21 @@ private RemotingCommand consumerSendMsgBack(final ChannelHandlerContext ctx, fin return response; } + // Make sure message store of the broker is writable. if (!PermName.isWriteable(this.brokerController.getBrokerConfig().getBrokerPermission())) { response.setCode(ResponseCode.NO_PERMISSION); response.setRemark("the broker[" + this.brokerController.getBrokerConfig().getBrokerIP1() + "] sending message is forbidden"); return response; } + // TODO: we should warn client here because OP may carelessly get things wrong here. — End diff – I think this needs another JIRA issue too.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          What's wrong with this PR. if no other polish, suggest close it

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 What's wrong with this PR. if no other polish, suggest close it
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          @vongosling I suggest to create JIRA issues for all TODOs and FIXMEs here to make sure they are fixed someday.
          It's not critical though, if you want to merge, please go ahead

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 @vongosling I suggest to create JIRA issues for all TODOs and FIXMEs here to make sure they are fixed someday. It's not critical though, if you want to merge, please go ahead
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lizhanhui commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/80

          Let's close this PR for now and merge parts of docs which helps.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lizhanhui commented on the issue: https://github.com/apache/incubator-rocketmq/pull/80 Let's close this PR for now and merge parts of docs which helps.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lizhanhui closed the pull request at:

          https://github.com/apache/incubator-rocketmq/pull/80

          Show
          githubbot ASF GitHub Bot added a comment - Github user lizhanhui closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/80
          Hide
          zander dongeforever added a comment -

          Github user lizhanhui closed the pull request at:
          https://github.com/apache/incubator-rocketmq/pull/80

          Show
          zander dongeforever added a comment - Github user lizhanhui closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/80

            People

            • Assignee:
              vongosling vongosling
              Reporter:
              lizhanhui Zhanhui Li
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development