support for auth enabled Marathon #4
Conversation
| this.logger = logger; | ||
| try { | ||
| MarathonServiceDiscoveryHelper.start(marathonEndpoint, appId, port, logger); | ||
| MarathonServiceDiscoveryHelper.start(marathonEndpoint, appId, port, marathonUsername, marathonPassword, | ||
| logger); | ||
| } catch (Exception e) { | ||
| logger.severe("Failed to start service discovery!", e); |
There was a problem hiding this comment.
Is it ok to just log the exception? What is the client expectation?
There was a problem hiding this comment.
Service startup should not be impacted if marathon discovery fails so I guess that's why we are not rethrowing the Exception here.
| this.logger = logger; | ||
| try { | ||
| MarathonServiceDiscoveryHelper.start(marathonEndpoint, appId, port, logger); | ||
| MarathonServiceDiscoveryHelper.start(marathonEndpoint, appId, port, marathonUsername, marathonPassword, |
There was a problem hiding this comment.
Will this work if someone has upgraded the hazelcast marathon client but the marathon cluster isn't still not auth enabled?
There was a problem hiding this comment.
yes, if marathon username or password is null or empty we do not make auth based call inside the start method
| HazelcastInstance hazelcast1 = getHazelcastInstance(5701); | ||
| HazelcastInstance hazelcast2 = getHazelcastInstance(5702); | ||
| HazelcastInstance hazelcast3 = getHazelcastInstance(5703); | ||
| HazelcastInstance hazelcast1 = getHazelcastInstanceWithoutBasicAuth(5701); |
There was a problem hiding this comment.
Let's have test cases for both with and without auth
There was a problem hiding this comment.
yes, if you see here 87 and 88 line one's are Without Auth instances and 89th line one is With Auth. And in 91st line asserting that cluster members should be 3
No description provided.