ActiveMQ and JMS broker - [merged] #146

Closed jdonkervliet closed 4 years ago

In GitLab by @larsdetombe on May 19, 2020, 15:22

Merges feature/activemq-broker -> development

Belongs to issues #11 #12

This merge request adds a JMS broker on which ActiveMQ can run.

In GitLab by @larsdetombe on May 19, 2020, 15:25

changed title from {-Feature/activemq-} broker to {+ActiveMQ and JMS+} broker

In GitLab by @larsdetombe on May 19, 2020, 15:25

changed the description

In GitLab by @wubero on May 19, 2020, 15:37

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 136

Shouldn't there be a whiteline after this catch block? Since there is a finally block afterwards

In GitLab by @wubero on May 19, 2020, 15:39

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 158

Shouldn't there be a whiteline before this if? Change this or the method above so it remains consistent

In GitLab by @wubero on May 19, 2020, 15:39

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 182

Whiteline before this if

In GitLab by @swabbur on May 19, 2020, 15:40

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 136

Correct, there should be.

In GitLab by @larsdetombe on May 19, 2020, 15:44

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 158

My thinking here was that the topic subscribers were connected to the if statement, but do we want a whiteline before every if statement then?

In GitLab by @swabbur on May 19, 2020, 15:45

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 158

If they are directly related, I'm fine with keeping them together.

In GitLab by @larsdetombe on May 19, 2020, 15:52

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 182

My thinking here was also that the code before the if is related to the actual if statement

In GitLab by @swabbur on May 19, 2020, 16:15

Commented on pom.xml line 47

Do we need a dependency for JMS as well? (I'm not sure whether Java EE is included in all JDKs and JREs by default)

In GitLab by @swabbur on May 19, 2020, 16:17

Commented on src/main/java/net/glowstone/messaging/brokers/Brokers.java line 41

As the factory is shared between multiple ActiveMQ brokers, should't it be checked whether it already exists instead of replacing it?

Either make it a local variable or make sure that the factory is properly reused.

In GitLab by @swabbur on May 19, 2020, 16:18

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 76

The whiteline above this if-statement isn't required, but I don't mind leaving it in either.

In GitLab by @swabbur on May 19, 2020, 16:20

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 122

I would personally add a whiteline at the start of the try-block (as it contains multiple code blocks), and remove the whiteline at the start of the method as it only contains one code-block (the locking try-catch-finally).

In GitLab by @larsdetombe on May 19, 2020, 17:48

Commented on src/main/java/net/glowstone/messaging/brokers/Brokers.java line 41

changed this line in version 2 of the diff

In GitLab by @larsdetombe on May 19, 2020, 17:48

added 1 commit

d52886b2 - Add dependancy and fix codestyle

Compare with previous version

In GitLab by @larsdetombe on May 19, 2020, 17:49

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 136

I hope I added the whiteline where you wanted, if that is the case you can resolve this thread.

In GitLab by @larsdetombe on May 19, 2020, 17:49

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 158

I decided to add the whiteline to stay consistent

In GitLab by @larsdetombe on May 19, 2020, 17:50

Commented on pom.xml line 47

I believe it is not included in all JDKs by default, therefore I added JMS as a dependancy.

In GitLab by @larsdetombe on May 19, 2020, 17:51

Commented on src/main/java/net/glowstone/messaging/brokers/Brokers.java line 41

I decided to create a local variable, because when multiple brokers are created the uri might be different for each broker in which case a local variable might be better.

In GitLab by @larsdetombe on May 19, 2020, 17:51

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 76

I decided to keep the whiteline here to stay more consistent

In GitLab by @larsdetombe on May 19, 2020, 17:51

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 122

done

In GitLab by @larsdetombe on May 19, 2020, 17:53

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 182

In the end I did decide to add a whiteline to stay consistent.

In GitLab by @swabbur on May 19, 2020, 18:00

resolved all threads

In GitLab by @wubero on May 19, 2020, 18:14

resolved all threads

In GitLab by @JimVliet on May 19, 2020, 18:21

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 20

'a connection' is a little too vague in my opinion, can you expand on that?

In GitLab by @JimVliet on May 19, 2020, 18:35

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 125

You can use computeIfAbsent here:

In GitLab by @larsdetombe on May 19, 2020, 18:35

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 20

changed this line in version 3 of the diff

In GitLab by @larsdetombe on May 19, 2020, 18:35

added 1 commit

8b3e7005 - Expand JmsBroker javadoc

Compare with previous version

In GitLab by @larsdetombe on May 19, 2020, 18:36

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 20

I expanded the javadoc

In GitLab by @JimVliet on May 19, 2020, 18:37

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 30

Perhaps we can change the generic type name of Topic to signify the difference with the JmsTopics. Perhaps BrokerTopic?

In GitLab by @larsdetombe on May 19, 2020, 18:40

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 30

I think the generic type Topic should stay consistent. This way people know it is the same Topic that is used in other classes.

In GitLab by @JimVliet on May 19, 2020, 18:54

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 30

It is not really the same "Topic", just a generic type that can basically be anything.

In GitLab by @JimVliet on May 19, 2020, 18:58

How did the activeMQ broker perform compared to the broker we have right now? I am just curious.

In GitLab by @JimVliet on May 19, 2020, 19:03

Commented on src/main/java/net/glowstone/messaging/brokers/Brokers.java line 23

It seems you missed a return value here.

In GitLab by @JimVliet on May 19, 2020, 19:06

Commented on src/main/java/net/glowstone/messaging/brokers/Brokers.java line 38

The JmsBroker that is returned will be raw here, without any of the generic types associated with it. This means that you always have to cast it after creating it. So it would be nice if this method could return the JmsBroker immediately with the correct type.

In GitLab by @JimVliet on May 19, 2020, 19:13

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 55

The createSession documenation specifies the following:

This method has been superseded by the method createSession(int sessionMode) which specifies the same information using a single argument, and by the method createSession() which is for use in a Java EE JTA transaction. Applications should consider using those methods instead of this one.

So we should probably look into using that.

Furthermore, can you perhaps place a comment as to what the reason is for choosing that session mode and not one of the others?

In GitLab by @JimVliet on May 19, 2020, 19:16

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsSubscriber.java line 15

The consumer field is never used (only assigned). So what is the reason for it being here?

In GitLab by @JimVliet on May 19, 2020, 19:19

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 135

Would it not be possible to use the contains method directly? Currently the hashcode of sub is only based on the subscriber anyway.

In GitLab by @julian9499 on May 19, 2020, 19:33

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 36

I find this very hard to distinguish this from a single jmsTopic, maybe call it a jmsTopicMap.

In GitLab by @JimVliet on May 19, 2020, 19:34

Commented on src/main/java/net/glowstone/messaging/brokers/jms/serializers/ProtocolCodec.java line 73

You should be able to use bytesMessage.readBytes(bytes) here, since you are trying to read the maximum amount of bytes possible anyway.

In GitLab by @julian9499 on May 19, 2020, 19:35

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 156

Same here. Try to avoid multiple with an s at the end of the word. It is really similar to the singular word.

In GitLab by @julian9499 on May 19, 2020, 19:38

Commented on src/test/java/net/glowstone/messaging/brokers/jms/JmsBrokerTest.java line 48

Why are warnings suppressed here?

In GitLab by @julian9499 on May 19, 2020, 19:39

Commented on src/test/java/net/glowstone/messaging/brokers/jms/JmsBrokerTest.java line 119

Rename this to createproducer

In GitLab by @JimVliet on May 19, 2020, 19:53

Commented on src/main/java/net/glowstone/messaging/brokers/jms/serializers/ProtocolCodec.java line 84

Would it be possible to avoid copying the data over? Currently, we are first writing to a bytes array. After which we immediately write it to the ByteBuf, which seems redundant.

After a little bit of research I found out that we can use Unpooled.wrappedBuffer(<bytes array here>). This basically uses the given array as buffer. This should be okay, since nothing should be written to the buffer during decoding.

In GitLab by @swabbur on May 19, 2020, 20:00

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 125

Correct, you could. But, if I remember correctly, the calls made inside the if-statement can throw an exception, which would require a lot of additional code to handle properly when thrown inside of a lambda expression.

If not, it might still be preferable iff its the case in the other methods (as keeping things consistent might be more important).

In GitLab by @swabbur on May 19, 2020, 20:03

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 30

Except that it is "supposed" to be the same as used by the other classes (which are all meant to be used together). Also, it matches the interface's generic type name, which I think is better than having it renamed.

Don't get me wrong, I do agree that having to use jms.Broker is aweful!

In GitLab by @swabbur on May 19, 2020, 20:04

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsSubscriber.java line 15

I think not storing it could lead to it being garbage collected. Which would be bad, as we never have to call it, but it does keep track of the callback we've set up.

In GitLab by @JimVliet on May 19, 2020, 20:05

Commented on src/main/java/net/glowstone/messaging/brokers/jms/serializers/ProtocolCodec.java line 56

The getBytes method starts writing the data from its buffer to bytes at the specified index. This means that the front of bytes will be empty up to index. I am pretty sure this is a bug.

In GitLab by @swabbur on May 19, 2020, 20:06

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 36

Including the type in the name of a variable is often considered bad practice as it requires renaming the variable whenever the type changes (it's a form of code-duplication).

In GitLab by @swabbur on May 19, 2020, 20:07

Commented on src/main/java/net/glowstone/messaging/brokers/jms/JmsBroker.java line 156

I think that is mostly a problem of the English language haha

atlarge-research / opencraft

ActiveMQ and JMS broker - [merged] #146