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.

Sign up for GitHub

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

Jump to bottom

Component creation transparency #63

Open
wants to merge 7 commits into
base: pg-merge
Choose a base branch
from
Open

Component creation transparency #63

wants to merge 7 commits into from

Conversation

sourabhpoddar404
Copy link
Contributor

No description provided.

sourabhpoddar404 and others added 4 commits June 15, 2020 15:45
@sourabhpoddar404 @altafhusen-mr
@Yamini19 @melissadas
Implementation of Direct Container Creator
7d392de 
Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com>
Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com>
Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>
@sourabhpoddar404 @altafhusen-mr
@Yamini19 @melissadas
Changes to BenchmarkControllerTest
82bec47 
Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com>
Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com>
Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>
@sourabhpoddar404 @altafhusen-mr
@Yamini19 @melissadas
Added test case for new implementation
90873d2 
Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com>
Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com>
Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com>
Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>
@altafhusen-mr @sourabhpoddar404
@Yamini19 @melissadas
Changes for the component creation based on feedback
e14cc95 
Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com>
Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com>
Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com>
Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>
MichaelRoeder
MichaelRoeder requested changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@MichaelRoeder MichaelRoeder left a comment

Choose a reason for hiding this comment

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

I am not convinced that this implementation fulfills the goal. It is solely bound to the creation of data generators, task generators and evaluation storage. However, the transparency should hold for all creations of containers. What about the creation of a container in one of the other components? It is not covered at all.

Please make sure that you apply the formatting rules of the .editorconfig file. The formatting of the added lines is nearly always wrong.

src/main/java/org/hobbit/core/components/AbstractBenchmarkController.java Outdated
@@ -206,7 +214,7 @@ protected void createTaskGenerators(String taskGeneratorImageName, int numberOfT
* @param generatorIds
* set of generator container names
*/
private void createGenerator(String generatorImageName, int numberOfGenerators, String[] envVariables,
public void createGenerator(String generatorImageName, int numberOfGenerators, String[] envVariables,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain why you need to change the visibility of this method.

src/main/java/org/hobbit/core/components/AbstractCommandReceivingComponent.java Outdated
public void createDummyComponent(byte command, byte[] data, String sessionId, BasicProperties props) {
// TODO Auto-generated method stub

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we discuss that this method is not necessary in this abstract class? Or is there a reason why it is still here?

src/main/java/org/hobbit/core/components/AbstractCommandReceivingComponent.java Outdated
@@ -64,17 +64,17 @@
/**
* Name of this Docker container.
*/
private String containerName;
protected String containerName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have all the visibility modifiers been changed? It does not make sense to me. Please explain why it is necessary.

src/main/java/org/hobbit/core/components/AbstractTaskGenerator.java Outdated
@@ -132,6 +132,7 @@ public void init() throws Exception {
dataGenReceiver = DataReceiverImpl.builder().dataHandler(new DataHandler() {
@Override
public void handleData(byte[] data) {

Copy link
Contributor

Choose a reason for hiding this comment

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

If that is the only change in this class, it does not make sense to me to have it. Please try to avoid changes like that and revert the change here.

src/main/java/org/hobbit/core/containerservice/ContainerCreation.java
* @author altaf, sourabh, yamini, melisa
*
*/
public interface ContainerCreation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add javadoc comments to all methods of this interface.

src/main/java/org/hobbit/core/containerservice/DirectContainerCreator.java Outdated
abstractBenchmarkController.getResponseFutures().put(correlationId, containerFuture);
}
byte data[] = RabbitMQUtils.writeString(
abstractBenchmarkController.getGson().toJson(new StartCommandData(imageName, containerType, abstractBenchmarkController.getContainerName(), envVariables, netAliases)));
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this fit to your own implementation of a direct communication? 🤔

src/main/java/org/hobbit/core/containerservice/DirectContainerCreator.java Outdated
}
}
}catch(Exception e) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring an exception is not a good idea. If there is an error, it should be logged. Apart from that, the components reacted on a failed container creation in the previous implementations. This does not seem to be the case in this implementation. However, that is not good. A component needs to be able to recognize that the creation of a container has failed to react accordingly.

src/main/java/org/hobbit/core/containerservice/RabbitMQContainerCreator.java
* @author altaf, sourabh, yamini, melisa
*
*/
public class RabbitMQContainerCreator implements ContainerCreation {
Copy link
Contributor

Choose a reason for hiding this comment

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

The "DirectContainerCreator" seems to use RabbitMQ as well. So where is the difference?

src/test/java/org/hobbit/core/components/BenchmarkControllerTest.java Outdated
}

public void createDummyComponent(byte command, byte[] data, String sessionId, AMQP.BasicProperties props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The method name is not really helpful. Please try to find a name that states what the method does.

src/test/java/org/hobbit/core/components/dummy/DummyDataCreator.java Outdated
@@ -34,6 +34,7 @@ protected void generateData() throws Exception {
byte data[];
for (int i = 0; i < dataSize; ++i) {
data = RabbitMQUtils.writeString(Integer.toString(i));

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert changes like that which do not add any functionality but still could lead to merge conflicts, etc.

sourabhpoddar404 and others added 2 commits July 4, 2020 15:05
@sourabhpoddar404 @altafhusen-mr
@Yamini19 @melissadas
Feedback changes
a57e2e1 
Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com>
Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com>
Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com>
Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>
@melissadas
Added Javadocs for the components.
8a26f76
@MichaelRoeder MichaelRoeder changed the base branch from develop to pg-merge September 16, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelRoeder MichaelRoeder MichaelRoeder requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sourabhpoddar404 @MichaelRoeder @altafhusen-mr @melissadas