Java: validate initial hosts, configuration and nodes loaded from API #45
Conversation
dkropachev
force-pushed
the
dk/java-validate-hosts
branch
2 times, most recently
from
3159dc9 to
2a542a6
Compare
dkropachev
changed the title
dkropachev
force-pushed
the
dk/java-validate-hosts
branch
from
2a542a6 to
a0f4b6d
Compare
dkropachev
force-pushed
the
dk/java-validate-hosts
branch
2 times, most recently
from
59aacea to
23dfc62
Compare
|
@nyh, @Bouncheck , could you please take a look at it. |
| // A private constructor. Use the create() functions below instead, as | ||
| // they also start the thread after creating the object. |
There was a problem hiding this comment.
Seems no longer up to date. There should be comments about the usage of the new API.
Also should we be removing the old API? I don't know how many people use this and how big of a deal would that be.
There was a problem hiding this comment.
Removed, it is not a library, user jsut copy paste it, so no problem there.
| @@ -26,16 +27,21 @@ public class AlternatorLiveNodes extends Thread { | |||
| private final String alternatorScheme; | |||
| private final int alternatorPort; | |||
|
|
|||
| private final AtomicReference<List<String>> liveNodes; | |||
| private final AtomicReference<List<URI>> liveNodes; | |||
| private final List<String> initialNodes; | |||
| private final AtomicInteger nextLiveNodeIndex; | |||
|
|
|||
| private static Logger logger = Logger.getLogger(AlternatorLiveNodes.class.getName()); | |||
|
|
|||
| @Override | |||
| public void run() { | |||
| logger.log(Level.INFO, "AlternatorLiveNodes thread starting"); | |||
There was a problem hiding this comment.
With start() overridden I think this log line should be there instead. It's slightly misleading.
There was a problem hiding this comment.
Changed log message and added end message.
| return create(uri.getScheme(), Arrays.asList(uri.getHost()), uri.getPort()); | ||
|
|
||
| @Override | ||
| public void start() { |
There was a problem hiding this comment.
While I don't see any errors I'm not sure if it's a common practice to override start. May be confusing to read in the future, but I don't really know what's the general consensus about that.
There was a problem hiding this comment.
What would you recommend ?
There was a problem hiding this comment.
Can we leave it as it is ?
There was a problem hiding this comment.
I'd keep most of the initialization in constructor and run() and here I'd just log that the thread is starting.
There was a problem hiding this comment.
Can we leave it as it is ?
If it works then it's probably fine. I would not want to block further work
| liveNodes = AlternatorLiveNodes.create(seedURI); | ||
| liveNodes = new AlternatorLiveNodes(seedURI); | ||
| try { | ||
| liveNodes.validate(); |
There was a problem hiding this comment.
I think it would be nicer if user could not use AlternatorLiveNodes without passing validation. Someone working on this repo later may not notice that he should call .validate() after creation.
There was a problem hiding this comment.
Make sense, but AlternatorLiveNodes is auxiliary code, not supposed used by user directly.
I would not want to constructor or start to throw an exceptions, keep everything separate, so that it would be easier to test it later.
dkropachev
force-pushed
the
dk/java-validate-hosts
branch
from
23dfc62 to
cd255cc
Compare
| return create(uri.getScheme(), Arrays.asList(uri.getHost()), uri.getPort()); | ||
|
|
||
| @Override | ||
| public void start() { |
There was a problem hiding this comment.
I'd keep most of the initialization in constructor and run() and here I'd just log that the thread is starting.
| return create(uri.getScheme(), Arrays.asList(uri.getHost()), uri.getPort()); | ||
|
|
||
| @Override | ||
| public void start() { |
There was a problem hiding this comment.
Can we leave it as it is ?
If it works then it's probably fine. I would not want to block further work
| @@ -26,69 +27,115 @@ public class AlternatorLiveNodes extends Thread { | |||
| private final String alternatorScheme; | |||
| private final int alternatorPort; | |||
|
|
|||
| private final AtomicReference<List<String>> liveNodes; | |||
| private final AtomicReference<List<URI>> liveNodes; | |||
| private final List<String> initialNodes; | |||
There was a problem hiding this comment.
I'm late to review this PR, but I don't understand some of the changes here. Here in particular - why would we want to save initialNodes forever? We only use the initial list in the constructor, and that's it. Saving initialNodes forever - potentially a week after initialization - suggestion we have some logic that goes back to initialNodes (e.g., in case we lost all the nodes in the cluster, or something)... But - do we have such logic? We don't, as far as I know.
There was a problem hiding this comment.
We would want to postpone validation, since initial live nodes are strings and liveNodes are URIs.
In next PR we going to switch all of them to URIs, but we still need this validation, because we would want to make sure that URI to URL conversion works flawlessly, while URI is relax regarding scheme definition URL is more strict, so we would want to be sure that error is not happening at runtime, only on configuration stage.
There was a problem hiding this comment.
But why "postpone validation"? Why shouldn't the constructor validate its options? It is perfectly normal for a Java constructor to validate its parameters and throw an exception if they are invalid.
There was a problem hiding this comment.
I would say both cases normal, but having it separately allows you more flexibility, smaller bricks makes it easier to build what you want.
Say customer whants to reuse it in their repo, and they build a classes first and only then validate their configuration.
Now, if your class does validation when classes is being built, there is no good way to do that.
While if you have it separate, you can do it whenever you want.
Changes:
URI, not as stringAlternatorLiveNodes.startwhen it is createdAlternatorLiveNodes.validateto check if it is configured properly