Component creation transparency #63
Conversation
Co-authored-by: Altafhusen <[email protected]> Co-authored-by: Yamini <[email protected]> Co-authored-by: Melissa <[email protected]>
Co-authored-by: Altafhusen <[email protected]> Co-authored-by: Yamini <[email protected]> Co-authored-by: Melissa <[email protected]>
Co-authored-by: Sourabh <[email protected]> Co-authored-by: Altafhusen <[email protected]> Co-authored-by: Yamini <[email protected]> Co-authored-by: Melissa <[email protected]>
Co-authored-by: Sourabh <[email protected]> Co-authored-by: Altafhusen <[email protected]> Co-authored-by: Yamini <[email protected]> Co-authored-by: Melissa <[email protected]>
There was a problem hiding this comment.
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.
| @@ -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, | |||
There was a problem hiding this comment.
Please explain why you need to change the visibility of this method.
| public void createDummyComponent(byte command, byte[] data, String sessionId, BasicProperties props) { | ||
| // TODO Auto-generated method stub | ||
|
|
||
| } |
There was a problem hiding this comment.
Didn't we discuss that this method is not necessary in this abstract class? Or is there a reason why it is still here?
| @@ -64,17 +64,17 @@ | |||
| /** | |||
| * Name of this Docker container. | |||
| */ | |||
| private String containerName; | |||
| protected String containerName; | |||
There was a problem hiding this comment.
Why have all the visibility modifiers been changed? It does not make sense to me. Please explain why it is necessary.
| @@ -132,6 +132,7 @@ public void init() throws Exception { | |||
| dataGenReceiver = DataReceiverImpl.builder().dataHandler(new DataHandler() { | |||
| @Override | |||
| public void handleData(byte[] data) { | |||
|
|
|||
There was a problem hiding this comment.
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.
| * @author altaf, sourabh, yamini, melisa | ||
| * | ||
| */ | ||
| public interface ContainerCreation { |
There was a problem hiding this comment.
Please add javadoc comments to all methods of this interface.
| abstractBenchmarkController.getResponseFutures().put(correlationId, containerFuture); | ||
| } | ||
| byte data[] = RabbitMQUtils.writeString( | ||
| abstractBenchmarkController.getGson().toJson(new StartCommandData(imageName, containerType, abstractBenchmarkController.getContainerName(), envVariables, netAliases))); |
There was a problem hiding this comment.
How does this fit to your own implementation of a direct communication? 🤔
| } | ||
| } | ||
| }catch(Exception e) { | ||
|
|
There was a problem hiding this comment.
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.
| * @author altaf, sourabh, yamini, melisa | ||
| * | ||
| */ | ||
| public class RabbitMQContainerCreator implements ContainerCreation { |
There was a problem hiding this comment.
The "DirectContainerCreator" seems to use RabbitMQ as well. So where is the difference?
| } | ||
|
|
||
| public void createDummyComponent(byte command, byte[] data, String sessionId, AMQP.BasicProperties props) { |
There was a problem hiding this comment.
The method name is not really helpful. Please try to find a name that states what the method does.
| @@ -34,6 +34,7 @@ protected void generateData() throws Exception { | |||
| byte data[]; | |||
| for (int i = 0; i < dataSize; ++i) { | |||
| data = RabbitMQUtils.writeString(Integer.toString(i)); | |||
|
|
|||
There was a problem hiding this comment.
Please revert changes like that which do not add any functionality but still could lead to merge conflicts, etc.
Co-authored-by: Sourabh <[email protected]> Co-authored-by: Altafhusen <[email protected]> Co-authored-by: Yamini <[email protected]> Co-authored-by: Melissa <[email protected]>
No description provided.