XD-1350: Remove enums as option coercion mechanism #619

Closed
wants to merge 4 commits into
from

3 participants

@ericbottard
Spring member

I'm obviously not a big fan of the empty xml files.

Alternatives (and also considering not using varargs in the constructor in favor of a more explicit call) :

{
   addValidValue("local");
   removeValidValue("memory");
}
@markfisher
Spring member

I don't think empty XML files are the way to go. Just accept that "local" is a valid option.

@markfisher
Spring member

A couple other comments:

  1. We do not necessarily need to rush to remove enums for "store" and "analytics". The most important first step is to support a pluggable message bus.
  2. The control transport option will be disappearing with our move to ZooKeeper for deployment.
@ericbottard
Spring member
@markpollack markpollack self-assigned this Mar 4, 2014
@markfisher markfisher commented on an outdated diff Mar 4, 2014
...erver/options/FromResourceLocationOptionHandlers.java
+public final class FromResourceLocationOptionHandlers {
+
+ private FromResourceLocationOptionHandlers() {
+
+ }
+
+ public static final String SINGLE_NODE_SPECIAL_CONTROL_TRANSPORT = "local";
+
+ /**
+ * Computes values for --controlTransport for the distributed case.
+ */
+ public static class DistributedControlTransportOptionHandler extends FromResourceLocationOptionHandler {
+
+ public DistributedControlTransportOptionHandler(CmdLineParser parser, OptionDef option, Setter<String> setter)
+ throws IOException {
+ super(parser, option, setter, "classpath*:/META-INF/spring-xd/transports/*-admin.xml");
@markfisher
Spring member
markfisher added a line comment Mar 4, 2014

we should probably at least use a private constant for the common prefix ("classpath*:/META-INF/spring-xd") so we don't duplicate that for each of these

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ericbottard
Spring member

Rebased and extracted constant for config file location

@markfisher markfisher commented on the diff Mar 7, 2014
...amework/xd/dirt/server/options/SingleNodeOptions.java
- @Option(name = "--transport", usage = "The transport to use for data messages (from node to node)")
- private DataTransport transport;
+ @Option(name = "--transport", handler = SingleNodeDataTransportOptionHandler.class,
+ usage = "The transport to use for data messages (from node to node)")
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

I know this did not change in this PR, but saying "node to node" in the context of single node does not make sense.

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

Indeed. Do you want me to change it, or can you do on merge?

@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

I can do that on merge. I also created a new issue: https://jira.springsource.org/browse/XD-1377

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff Mar 7, 2014
...server/options/FromResourceLocationOptionHandler.java
+
+import org.springframework.core.io.Resource;
+import org.springframework.core.io.support.PathMatchingResourcePatternResolver;
+
+
+/**
+ * An {@link OptionHandler} that scans a resource pattern for existing resources (using a single '*' wildcard) and
+ * allows all String values <i>s</i> that would fit if that single wildcard was replaced by <i>s</i>.
+ *
+ * <p>
+ * Given that an option handler has to appear as an annotation parameter, expected usage is to sublcass this class,
+ * provide the canonical 3 arg contructor to {@link OptionHandler} and pass the resource pattern as the 4th argument.
+ * </p>
+ *
+ */
+public class FromResourceLocationOptionHandler extends OptionHandler<String> {
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

If you don't mind, I think I'll rename this to ResourcePatternScanningOptionHandler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff Mar 7, 2014
...erver/options/FromResourceLocationOptionHandlers.java
+ *
+ * @author Eric Bottard
+ */
+public final class FromResourceLocationOptionHandlers {
+
+ private FromResourceLocationOptionHandlers() {
+
+ }
+
+ private static final String CONFIGURATION_ROOT = "classpath*:/META-INF/spring-xd/";
+
+ /**
+ * The special controlTransport, usable only in singlenode mode, that requires the admin and container application
+ * contexts to talk to each other.
+ */
+ public static final String SINGLE_NODE_SPECIAL_CONTROL_TRANSPORT = "local";
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

If you don't mind, I think I'll rename this constant to SINGLE_NODE_LOCAL_CONTROL_TRANSPORT.

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

Definitely not, on both.

@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

Just to be clear, you mean that "you definitely DONT MIND"?... or "definitely do not change". I don't want to make you angry :)

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

DON'T YOU DARE CHANGE THOSE!

No, just kidding, be my guest of course. better names indeed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markpollack markpollack assigned markfisher and unassigned markpollack Mar 7, 2014
@markfisher
Spring member

Based on the discussion, I will apply some renames while merging.

Otherwise LGTM.

@markfisher markfisher commented on the diff Mar 7, 2014
...erver/options/FromResourceLocationOptionHandlers.java
+import org.kohsuke.args4j.OptionDef;
+import org.kohsuke.args4j.spi.Setter;
+
+
+/**
+ * Holds definitions of {@link FromResourceLocationOptionHandler}s used in Spring XD.
+ *
+ * @author Eric Bottard
+ */
+public final class FromResourceLocationOptionHandlers {
+
+ private FromResourceLocationOptionHandlers() {
+
+ }
+
+ private static final String CONFIGURATION_ROOT = "classpath*:/META-INF/spring-xd/";
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

Could use the constant for config root, e.g.: "classpath*:/" + ConfigLocations.XD_CONFIG_ROOT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff Mar 7, 2014
...server/options/FromResourceLocationOptionHandler.java
+import org.springframework.core.io.support.PathMatchingResourcePatternResolver;
+
+
+/**
+ * An {@link OptionHandler} that scans a resource pattern for existing resources (using a single '*' wildcard) and
+ * allows all String values <i>s</i> that would fit if that single wildcard was replaced by <i>s</i>.
+ *
+ * <p>
+ * Given that an option handler has to appear as an annotation parameter, expected usage is to sublcass this class,
+ * provide the canonical 3 arg contructor to {@link OptionHandler} and pass the resource pattern as the 4th argument.
+ * </p>
+ *
+ */
+public class FromResourceLocationOptionHandler extends OptionHandler<String> {
+
+ private Set<String> possibleValues = new HashSet<String>();
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

these 2 sets should be final

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher markfisher commented on the diff Mar 7, 2014
...erver/options/FromResourceLocationOptionHandlers.java
+
+ /**
+ * Computes values for --analytics in the distributed case (memory is NOT supported).
+ */
+ public static class DistributedAnalyticsOptionHandler extends FromResourceLocationOptionHandler {
+
+ public DistributedAnalyticsOptionHandler(CmdLineParser parser, OptionDef option, Setter<String> setter)
+ throws IOException {
+ super(parser, option, setter, CONFIGURATION_ROOT + "analytics/*-analytics.xml");
+ exclude("memory");
+ }
+
+ }
+
+ /**
+ * Computes values for --store in the distributed case (memory is NOT supported).
@markfisher
Spring member
markfisher added a line comment Mar 7, 2014

This says memory is NOT supported, but it is not excluded here.

Also it says for "distributed case", but this one appears to be applied to both distributed AND singlenode.

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

Indeed, there should be two variants of this one.

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

I take that back. Memory is supported for distributed as well. The javadoc is wrong that's all

@ericbottard
Spring member
ericbottard added a line comment Mar 7, 2014

Hmmm, now I have a doubt. Should check with current behavior, but I think there are actually two cases here indeed, because of the runtime container info (ie the admin is no longer the only thing that writes to the "store")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markfisher
Spring member

Actually I made a few more comments. Some can be handled on merge by me, but I need a reply on this one: https://github.com/spring-projects/spring-xd/pull/619/files#r10392789

@ericbottard
Spring member

OK, I finally found the problem that was blocking me and fixed it.
I opted with the following regarding "store", which is slightly different from what we have currently:
singlenode : redis + memory
distributed : redis only

The rationale is this: Although IMO only the admin server should be responsible for STORing, currently the container nodes do a little bit of that when they advertise their container id. Hence, the only case where this is fully functional is when using redis. One could argue that if you're not interested in the container info, then memory for distributed is fine.
In any case, as discussed earlier, this is going away soon, so..

@markfisher
Spring member

Yes, the Container nodes advertising their attributes (including the Container IDs) will all be moved to ZooKeeper.

@markfisher
Spring member

Here are the changes I plan to make while merging:

  1. in SingleNodeOptions: remove "node to node" comments
  2. make FromResourceLocationOptionHandler abstract
  3. in FromResourceLocationOptionHandlers: use "classpath*:/" + ConfigLocations.XD_CONFIG_ROOT
  4. in FromResourceLocationOptionHandler: the 2 sets ("*Values") should be final
  5. rename: FromResourceLocationOptionHandler -> ResourcePatternScanningOptionHandler
  6. rename: FromResourceLocationOptionHandlers -> ResourcePatternScanningOptionHandlers
  7. rename: SINGLE_NODE_SPECIAL_CONTROL_TRANSPORT -> SINGLE_NODE_LOCAL_CONTROL_TRANSPORT
@ericbottard
Spring member
@markfisher markfisher closed this Mar 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment