Skip to content

Issue #450. First commit. Queue Based Load Leveling #521

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

Merged
merged 9 commits into from
Nov 28, 2016

Conversation

Amarnath510
Copy link
Contributor

I have completed coding, design (UML), README.md. jUnit tests.
This is my first code submission for open source.
Please review and let me know the issues.

I have one question,
In the README.md file we have to generate the "pumlid". I don't know how to generate the same.
As of now I have left it blank. Let me know how to generate the same.

@iluwatar
Copy link
Owner

@Amarnath510 the Travis CI build is failing and needs to be fixed first. Looks like there's wrong artifactId in queue-load-leveling/pom.xml.

@Amarnath510
Copy link
Contributor Author

@iluwatar
I have made changes in queue-load-leveling/pom.xml file and pushed.
Should I again do a pull request or my changes are visible to you?

@iluwatar
Copy link
Owner

@Amarnath510 thanks for the quick fix. The changes are applied automatically as you push. Unfortunately now there's a new (Checkstyle) error in the build - see https://travis-ci.org/iluwatar/java-design-patterns/builds/177451598

@Amarnath510
Copy link
Contributor Author

Amarnath510 commented Nov 20, 2016

@iluwatar
I have updated all the checkstyle issues by looking at the error log.
Out of curiosity, how to get the issues produced in my development environment locally, that is in my Eclipse IDE? If these issues are coming in my environment I can fix them there itself.
FYI, I have given checkstyle.xml as the default one. Just the way it was mentioned here.

@iluwatar
Copy link
Owner

You can reproduce the Checkstyle issues in Eclipse IDE by selecting the project(s) in package/project explorer, right-clicking the project and choosing Checkstyle - Check Code with Checkstyle. I'll add that to the instructions.

@Amarnath510
Copy link
Contributor Author

Amarnath510 commented Nov 21, 2016

@iluwatar
Thanks for the update.
But the issue now is different.

For more details see: /home/travis/build/iluwatar/java-design-patterns/queue-load-leveling/target/pmd.xml -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-pmd-plugin:3.6:check (default) on project queue-load-leveling: You have 1 PMD violation.

Just to make clear what steps I have done,

  1. Forked the project and cloned it.
  2. Installed Maven in Eclipse (Neon.1a Release (4.6.1)) and Java Verison 1.8.
  3. Imported Existing Maven project by pointing to the root folder.
  4. Done all the Checkstyle related stuff.
  5. Right click on java-design-pattern in the project explorer - new - other - maven (in filer) - Maven Module
  6. Create new module, queue-load-leveling
  7. Created required Java files and UML files and README.md file.
  8. Updated pom.xml file with builder/pom.xml file contents and update artifactid to queu-load-leveling

Please let me know if I have missed something here.
I am not able to figure out the issue.

@iluwatar
Copy link
Owner

There is maven-pmd-plugin configured in the build and it has detected an error in the project. Scan the build output for more details about the PMD error and fix that.

@Amarnath510
Copy link
Contributor Author

Amarnath510 commented Nov 22, 2016

@iluwatar
How can I check this file,
For more details see: /home/travis/build/iluwatar/java-design-patterns/queue-load-leveling/target/pmd.xml

Also can you tell me how to compile the whole project locally. I want to check locally before committing my changes.

@iluwatar
Copy link
Owner

If you scroll the build output a bit upwards you will find this line: PMD Failure: org.queue.load.leveling.Task:29 Rule:UnusedModifier Priority:3 Avoid modifiers which are implied by the context.

To build locally you just need to do mvn clean install on the project root.

@Amarnath510
Copy link
Contributor Author

Amarnath510 commented Nov 23, 2016

@iluwatar
Thanks for pointing out the error. After solving all the issues I understood the project flow.
Please review my code and let me know if I need to make any changes.

Copy link
Owner

@iluwatar iluwatar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest some improvements to the code. In general the pattern looks good already 👍


## Credits

* [Design Pattern: Queue-Based Load Leveling Pattern](https://msdn.microsoft.com/en-us/library/dn589783.aspx)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would formulate this Microsoft Cloud Design Patterns: Queue-Based Load Leveling Pattern

srvExec.start();
} catch (Exception e) {
LOGGER.error(e.getMessage());
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of working directly with Threads it is recommended practise to use ExecutorService instead. For example take a look at this tutorial.

public String toString() {
return msg;
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would refactor to make this class immutable. Remove no-args constructor and setter and make msg final.


private static final Logger LOGGER = LoggerFactory.getLogger(App.class);

private BlockingQueue<Message> blkQueue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final


private static final Logger LOGGER = LoggerFactory.getLogger(App.class);

private MessageQueue msgQueue;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

private MessageQueue msgQueue;

// Total message count that a TaskGenerator will submit.
private int msgCount;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make attributes final

Copy link
Contributor Author

@Amarnath510 Amarnath510 Nov 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using msgCount to create particular number of messages which I am submitting to MessageQueue. For each message I am reducing the the msgCount value. If I make this value final then I can't keep count of message that I am creating.
Should I use a local variable and copy the value of the msgCount and then use it? Is this the way to do?

Apart from this I have made all the remaining changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok then. Can you resolve the merge conflict, please?

while (this.msgCount > 0) {
String statusMsg = "Message-" + this.msgCount + " submitted by " + Thread.currentThread().getName();
Message newMessage = new Message(statusMsg);
this.submit(newMessage);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message could be created and submitted on one line

import org.junit.Test;

/**
* Tests that Caching example runs without errors.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix comment

Message msg = new Message("MessageQueue Test");

// submit message
msgQueue.submitMsg(msg);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message can be created and submitted on one line

@Amarnath510
Copy link
Contributor Author

Amarnath510 commented Nov 27, 2016

@iluwatar
May be this questions seems to be too silly. Normally when I use TortoiseSVN, if there is a conflict I usually do code update and resolve the conflicts.
But in Git I don't have any knowledge on how to fix the below conflict. I am worried that I might screw up the code base. Could you please help me with steps about how to resolve this?

@iluwatar
Copy link
Owner

The conflict should be easy to resolve. See this guide for instructions how to solve this using the command line only.

@Amarnath510
Copy link
Contributor Author

@iluwatar
I have fixed the conflict. Thanks for the info.
I have made all the necessary changes after review. Please have a look.

@iluwatar iluwatar merged commit efc2c88 into iluwatar:master Nov 28, 2016
@iluwatar
Copy link
Owner

Good job @Amarnath510 👍 I'm happy to accept your contribution.

@Amarnath510
Copy link
Contributor Author

@iluwatar
👍 👍 👍
Thanks a lot. This is my first contribution to open source and I am really excited.
Grateful for your help at every stage. Also will write the required blog.
Excited to pick up the new pattern. Will do that after few days.

@iluwatar
Copy link
Owner

Excellent, I'm looking forward to hearing from you again 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants