Communication channel transparency #60

altafhusen-mr · 2020-06-21T19:18:10Z

No description provided.

Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com> Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com> Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com> Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>

…r clean up, RabbitMQ dependency code removal for DataGeneratorTest Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com> Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com> Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com> Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>

Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com> Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com> Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com> Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>

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>

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>

and Sending properties to DirectChannel Co-authored-by: Altafhusen <57062112+altafhusen-mr@users.noreply.github.com> Co-authored-by: Sourabh <56801226+sourabhpoddar404@users.noreply.github.com> Co-authored-by: Yamini <29372422+Yamini19@users.noreply.github.com> Co-authored-by: Melissa <21376912+melissadas@users.noreply.github.com>

for both DirectChannel and RabbitMQChannel, and code cleanup 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>

RabbitMQChannel 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>

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>

RabbitMQChannel

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

The implementation mainly covers what we discussed. However, I am not sure whether it really fulfills all the functionality that is necessary. In any case, there are several things that need to be improved. Please have a look at the following points as well as the comments in the code.

The files are not correctly formatted. Please follow the rules defined in the .editorconfig file.
It seems like a lot of the "direct communication" classes are missing a lot of implementations. Several methods are simply empty although they should have a functionality.
In several places, the error handling is not good. Instead of returning an error state (e.g., null), an object that is not helpful is returned and the occurred error is "hidden". This needs to be changed! I am not sure whether I found all of the faulty catch blocks so please double check them.
Some package names are not really clear to me. Maybe you could move some of the classes:

org.hobbit.core.data.handlers.DataHandler
org.hobbit.core.data.handlers.DataReceiver
org.hobbit.core.data.handlers.DataSender
org.hobbit.core.components.commonchannel.CommonChannel

should be moved to

org.hobbit.core.com

org.hobbit.core.components.channel.Direct*

should be moved to

org.hobbit.core.com.java

MichaelRoeder · 2020-06-22T16:05:01Z

src/main/java/org/hobbit/core/components/AbstractEvaluationModule.java

+    		return delivery.getBody();
+    	}
+    	return new byte[] {};
+    }


What exactly is that? Does it mean that the AbstractEvaluationModule does not work with a "direct" communication?

MichaelRoeder · 2020-06-22T16:06:18Z

src/main/java/org/hobbit/core/components/AbstractEvaluationModule.java

-            evalModule2EvalStoreQueue.channel.basicPublish("", evalModule2EvalStoreQueue.name, props, requestBody);
-            QueueingConsumer.Delivery delivery = consumer.nextDelivery();
+        	props = new BasicProperties.Builder().deliveryMode(2).replyTo(getFactoryForIncomingDataQueues().getQueueName(this)).build();
+        	getFactoryForOutgoingDataQueues().writeBytes(requestBody, "", getFactoryForOutgoingDataQueues().getQueueName(this), props);


Shouldn't the creation of properties etc. be part of a different method? What happens if a "direct" communication is used?

MichaelRoeder · 2020-06-22T16:09:35Z

src/main/java/org/hobbit/core/components/AbstractComponent.java

+        incomingDataQueueFactory = new ChannelFactory().getChannel(isRabbitMQEnabled(),
+                "", connectionFactory);
+        outgoingDataQueuefactory = new ChannelFactory().getChannel(isRabbitMQEnabled(),
+                "", connectionFactory);


Why there are three different instances of a ChannelFactory created? Why not simply create one and reuse it?

MichaelRoeder · 2020-06-22T16:45:25Z

src/main/java/org/hobbit/core/components/commonchannel/CommonChannel.java

+ * @author altaf, sourabh, yamini, melisa
+ *
+ */
+public interface CommonChannel {


It would be better to give it a name like Channel. It is unclear where the term common comes from.

In addition, Javadoc comments for the methods defined in this interface are missing.

MichaelRoeder · 2020-06-22T17:02:51Z

src/main/java/org/hobbit/core/components/channel/DirectCallback.java

+ * @author altaf, sourabh, yamini, melisa
+ *
+ */
+public class  DirectCallback {


Why is this a class? It should be either an interface or at least an abstract class.

MichaelRoeder · 2020-06-22T18:00:33Z

src/main/java/org/hobbit/core/components/channel/ReadByteChannel.java

+	       clone.put(original);
+	       original.rewind();
+	       clone.flip();
+	       return clone;


Why is the cloning so complicated? You can get direct access to the byte array of the buffer, copy it and wrap it with a new buffer.

MichaelRoeder · 2020-06-22T18:01:43Z

src/main/java/org/hobbit/core/components/channel/ReadByteChannel.java

+        		while(pipeChannel.getPipe().source().isOpen() && 
+        				pipeChannel.getPipe().source().read(buffer) > 0){
+        			threadPool.execute(new ProcessCallback(callback, clone(buffer), pipeChannel.getProps()));
+        			buffer.clear();


Wouldn't it be easier to execute buffer = ByteBuffer.allocate(1024); instead of cloning the buffer before clearing it?

MichaelRoeder · 2020-06-22T18:03:54Z

src/main/java/org/hobbit/core/components/channel/ReadByteChannel.java

+		        --i;
+		    }
+		    return Arrays.copyOf(inputArray, i + 1);
+		}


This is too complex. The method copies the complete array before creating a smaller array and copying the content into the smaller array. Apart from that, the search for the end does not make sense to me. What happens if the last bytes of the data are 0 ?

Please fix this method.

MichaelRoeder · 2020-06-22T18:09:34Z

src/main/java/org/hobbit/core/components/channel/ReadByteChannel.java

+ * @author altaf, sourabh, yamini, melisa
+ *
+ */
+public class ReadByteChannel extends DirectChannel implements Runnable{


It seems like this class is never used, or am I missing something?

ReadByteChannel is used in readBytes() method of DirectChannel class.

MichaelRoeder · 2020-06-22T18:11:17Z

src/main/java/org/hobbit/core/components/AbstractEvaluationStorage.java

+			        .createDefaultRabbitQueue(generateSessionQueueName(queueName));
+		} catch (IOException e) {
+			// TODO Auto-generated catch block
+			e.printStackTrace();


What about error handling? Here, the wrong object will be returned while the method should return null.

… Communication_channel_transparency � Conflicts: � src/main/java/org/hobbit/core/components/AbstractComponent.java

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>

removal, Test class revert changes. 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>

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 and others added 20 commits May 3, 2020 17:50

Channel transparency : DirectChannel implementation

d61f495

Remove comment from catch block

5ebdaf3

Fixing All the tests except SequencingTaskGeneratorTest for

21c4c40

RabbitMQChannel

MichaelRoeder requested changes Jun 22, 2020

View reviewed changes

altafhusen-mr and others added 5 commits June 24, 2020 16:45

Merge branch 'develop' of https://github.com/hobbit-project/core into…

6523996

… Communication_channel_transparency � Conflicts: � src/main/java/org/hobbit/core/components/AbstractComponent.java

Added Javadocs for the components.

3cb1680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Communication channel transparency #60

Communication channel transparency #60

altafhusen-mr commented Jun 21, 2020

MichaelRoeder left a comment

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

MichaelRoeder Jun 22, 2020

altafhusen-mr Jul 5, 2020

MichaelRoeder Jun 22, 2020

Communication channel transparency #60

Are you sure you want to change the base?

Communication channel transparency #60

Conversation

altafhusen-mr commented Jun 21, 2020

MichaelRoeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment