Broker configuration - [merged] #172

Closed jdonkervliet closed 4 years ago

In GitLab by @swabbur on Jun 4, 2020, 15:33

Merges feature/broker-config -> development

Resolves issue #101.

It was currently not possible to configure the broker via the config file. Now, the broker factory accepts a configuration class that can be build from the config file to build any broker the server could use.

In GitLab by @swabbur on Jun 4, 2020, 15:36

added 1 commit

14072e68 - Add missing whiteline before class definition

Compare with previous version

In GitLab by @julian9499 on Jun 4, 2020, 15:55

Commented on src/main/java/net/glowstone/util/config/ServerConfig.java line 394

Wouldn't it be handy to document the options here?

In GitLab by @swabbur on Jun 4, 2020, 17:54

Commented on src/main/java/net/glowstone/util/config/ServerConfig.java line 394

None of the other settings is explained here. I think it would be better to add the explanation to the ReadMe once we are ready to write instructions on how to install, configure, and use the server as a whole.

In GitLab by @JimVliet on Jun 4, 2020, 19:49

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

In my opinion it'd be better to just stick to lower case letters only. Perhaps just convert the type to lower case before the switch statement.

In GitLab by @JimVliet on Jun 4, 2020, 20:32

Working configurations for ActiveMQ and RabbitMQ:

    broker:
        type: ActiveMQ
        host: localhost
        port: 61616
        virtualHost: ''
        username: ''
        password: ''
        channel:
            type: Unsafe
            parallelismThreshold: 4

    broker:
        type: RabbitMQ
        host: localhost
        port: 5672
        virtualHost: '/'
        username: 'guest'
        password: 'guest'
        channel:
            type: Unsafe
            parallelismThreshold: 4

In GitLab by @JimVliet on Jun 4, 2020, 20:43

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

My only issue with this code right now, is that we do not provide proper feedback to the user if their configuration is incorrect, besides just throwing these exceptions. Have you looked into some ways that we can give the user some helpful feedback for configuring the server?

In GitLab by @julian9499 on Jun 5, 2020, 10:49

Commented on src/main/java/net/glowstone/util/config/ServerConfig.java line 394

I am fine with that as long as we do not forget anything due to time.

In GitLab by @swabbur on Jun 6, 2020, 16:46

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

We can add some validators in the ServerConfig. Currently, these only check whether the given values are of the correct type, but we could extend those to check whether the port is within a valid range, whether no illegal characters are present in the host address, and whether the broker and channel types actually exist. Would that be sufficient?

In GitLab by @swabbur on Jun 6, 2020, 16:47

Where would you suggest we add these? Should we provide these in the ReadMe? Or should we consider setting up a small Manual (e.g. in the etc/docs folder)?

In GitLab by @swabbur on Jun 6, 2020, 16:47

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

That would indeed be better, I'll change it right away.

In GitLab by @swabbur on Jun 6, 2020, 17:12

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

Oh... I just noticed the validators only reset the incorrect values to their defaults instead of notifying the user of their mistake.

In GitLab by @swabbur on Jun 6, 2020, 17:14

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

changed this line in version 3 of the diff

In GitLab by @swabbur on Jun 6, 2020, 17:14

added 1 commit

bdf8c0cb - Check lowercase version of configured broker and channel types

Compare with previous version

In GitLab by @swabbur on Jun 6, 2020, 17:15

Commented on src/main/java/net/glowstone/util/config/ServerConfig.java line 394

changed this line in version 4 of the diff

In GitLab by @swabbur on Jun 6, 2020, 17:15

added 1 commit

9d21250a - Improve config defaults

Compare with previous version

In GitLab by @JimVliet on Jun 7, 2020, 20:39

I think a README is still okay, since it hasn't become very large yet.

In GitLab by @JimVliet on Jun 7, 2020, 21:32

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

So is there anything else we can try? Or is that not trivial?

In GitLab by @swabbur on Jun 10, 2020, 15:24

added 69 commits

9d21250a...48f4db53 - 68 commits from branch development
67f1d895 - Merge development into broker-config

Compare with previous version

In GitLab by @swabbur on Jun 10, 2020, 15:27

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

I would personally prefer rewriting the ServerConfig to add this functionality, but that might be a little too much work. We could just throw an exception from the ServerConfig's validators, its a crude solution, but it would work.

In GitLab by @JimVliet on Jun 10, 2020, 16:54

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

If you can rewrite the ServerConfig, that'd be amazing. But since we are getting to a point where we need to prioritize where we spent our time, you might be able to spent your time more effectively elsewhere. Hence, why we could merge this if we cannot fix this in time.

In GitLab by @swabbur on Jun 10, 2020, 17:40

resolved all threads

In GitLab by @swabbur on Jun 10, 2020, 17:47

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

I'll take a look at rewriting the ServerConfig later, as it is not required at this moment.

In GitLab by @JimVliet on Jun 10, 2020, 17:48

merged

In GitLab by @JimVliet on Jun 10, 2020, 17:48

mentioned in commit 7b76b4d460cb3b372b062ddc37aa1604592bff84

atlarge-research / opencraft

Broker configuration - [merged] #172