STORM-2358: Update storm hdfs spout to remove specific implementation handling

[pasalkarsachin1]

As part of this change I have removed specific handling in code for TextFileReader & SequenceFileReader also made AbstractFileReader as public

[pasalkarsachin1]

Can someone take a look at this?

[arunmahadevan]

This assumes a ctor with expected number and type of params is available and tries to invoke it via reflection. Adding an init/open with required params in the FileReader is much cleaner IMO and avoids having to check and throw exceptions at runtime.

[pasalkarsachin1]

Same code is being there before my changes, I can create JIRA for this. ATM I am just trying to get rid of specific implementations

[arunmahadevan]

Not sure what you mean by "get rid of specific implementations". I see that you are mainly cleaning up and refactoring the code and so its better if you do it with this patch.

[arunmahadevan]

What is the need for this when the spout accepts the readerType via setReaderType ?

[pasalkarsachin1]

It was already there so I updated as per requirement.

[arunmahadevan]

1. How about backward compatibility ? Changing the signature will break existing clients. May be you can deprecate the earlier method and add a new one like setFileReader(Class<? extends FileReader> reader)

2. How would one initialize a HDFSSpout via Flux? Earlier since the method accepted a String it was pretty trivial. Try it out and add some example, or add an additional method like setFileReader(String className) that takes the class name.

[pasalkarsachin1]

Sure I will deprecate this. However adding deprecation will add more code

[pasalkarsachin1]

Done please review

[roshannaik]

Looks like you are removing support for aliases. Is that right ?

[pasalkarsachin1]

I have deprecated as per comments, not removed.

[roshannaik]

Overall this PR seems to be guided by misplaced ideas ... and complete disregard for backward compatibility.

[roshannaik]

Do you realize what you are doing here ?

[HeartSaVioR]

First of all, thanks for the contribution.

I'm seeing backward incompatible as @roshannaik stated, and like renaming class is not strictly necessary to. Huge part of maintaining open source is fighting with backward compatibility. What you change will affect number of developers. It's joyful thing for open source developers but also huge pain point.

Unless there are critical places so need to change or get rid of, breaking backward compatibility would not be accepted, and even it is really needed, it would be better to discuss widely.

[pasalkarsachin1]

@HeartSaVioR @roshannaik I have reverted class renaming. Other than this I don't see break in backward compatibility

[pasalkarsachin1]

@HeartSaVioR @roshannaik @arunmahadevan Can you review this? Let me know if there is any issue you see with backward compatibility.

[roshannaik]

Deprecating/removing aliasing support IS naturally a backward compat issue.

You have expressed your concern that you don't like registering aliases in HdfsSpout. But removing aliasing is not the solution to address your concern.

Like I said before, If you know how to implement aliasing without registering in HdfsSpout then it would address your concern and we can look into this PR.... otherwise there is nothing left to look into here.

[pasalkarsachin1]

@roshannaik Like I said even on Jira STORM-2358. I don't see point having code just to specifically handle alias. Code should be generic to handle all cases. If you want to push to have specific handling I don't think its right contribution & we already have this discussion on STORM-2358. I would like to see view from community.

Let me know from your point of view on backward compatibility. I know moving away from alias will have issues but to keep specific code just to support this is not worth

[HeartSaVioR]

@pasalkarsachin1 @roshannaik
Two things from me...

1. We should check that it works with Flux. In other words, if Flux doesn't support instantiating Class object and passing it, the change seriously breaks backward compatibility.

2. To be honest, accepting alias and class in one method doesn't look intuitive. Users should memorize the alias (TEXT, SEQ) to use. Instead of doing this, why not adding shortcut methods so that IDE can help for us? Like text(), sequence() or a bit more longer name if it doesn't feel enough. Other options we can create Enum for known implementations and full class name for others, but the former looks simpler unless we're aliasing too much.

cc. @arunmahadevan

ps. Keeping backward compatibility makes it bad here. If 1 is not true, we should keep method signature as it is, then we can't deprecate the alias usage even we want to get rid of it, since we're not changing method signature or even overload method here.

[arunmahadevan]

We should check that it works with Flux. In other words, if Flux doesn't support instantiating Class object and passing it, the change just breaks backward compatibility.

Agree and pointed out in one of the earlier comments. One suggestion was to have two methods,

1. setReader(Class<? extends FileReader> reader)
2. setReaderClass(String className)

To be honest, accepting alias and class in one method doesn't look intuitive. Users should memorize the alias (TEXT, SEQ) to use.

Right now the method accepts special strings "text", "seq" or class name, which looks confusing and not type safe. The proposed patch addresses this by accepting a FileReader class. I am not sure why cant we directly pass the FileReader implementation instead of the class (like setReader(FileReader reader)) and I believe flux will be able to handle this.

The current FileReader implementation is expected to have constructor with a specific signature, which cannot be enforced at compile time. IMO, this should also be fixed along with this patch by introducing some init method in the FileReader interface.

Regarding backward compatibility, if we are fine to break it in the next major version (2.0) we can keep this out of 1.x branch.

[HeartSaVioR]

@arunmahadevan

I am not sure why cant we directly pass the FileReader implementation instead of the class (like setReader(FileReader reader)) and I believe flux will be able to handle this.

It enforces implementation to be Serializable, which HDFS classes (fields in FileReader implementations) are not. That seems why HdfsSpout instantiates FileReader in open method.

[roshannaik]

@HeartSaVioR
I am OK with that line of thinking of handling aliases, as you are looking to make it more intuitive to user. .. although we may not have a real problem to justify public interface changes.

However... the author's concern is quite the opposite ... it is not from a user perspective. He is trying to make code more generic (as he puts it) by removing aliasing ... because we need to make the alias known to Hdfs Spout.... Which means no enums, no text() or sequence() etc.

Side note: This approach of using aliases OR class names in the same place has precedence in other projects as well... Apache Flume uses this pattern a fair amount.

[HeartSaVioR]

@roshannaik
Interesting. Could you point out where Apache Flume uses that approach? Just curious. :)
Btw, if it's from configuration backed by text file, we might have no option. But I think there're still some options to choose since we're assigning it from codebase.

[roshannaik]

Clink on this link :
http://flume.apache.org/FlumeUserGuide.html
and search for 'class name'

in file based config also dual methods/settings can be used to keep aliases and class names separate just like in code.

[pasalkarsachin1]

For points made by @HeartSaVioR

For point 2, I have various implementations in mind to add to open source one of them is implemented by megha10 for ZipTextFileReader another can be anySepratorReader like (pipe,comma). If we add enum for every implementation contributor has to do other extra work to enable it rather just adding reader.

Also if user adds his class where he can add it? He has to provide FQCN which comes back to same implementation which I did.

[pasalkarsachin1]

The current FileReader implementation is expected to have constructor with a specific signature, which cannot be enforced at compile time. IMO, this should also be fixed along with this patch by introducing some init method in the FileReader interface.`

I am thinking to work on this after this patch we can have init() api which verifies things before it proceeds.

It enforces implementation to be Serializable, which HDFS classes (fields in FileReader implementations) are not. That seems why HdfsSpout instantiates FileReader in open method.`

Once we provide init method we are providing user a way to initialize the non Serializable member.

[HeartSaVioR]

@pasalkarsachin1

For point 2, I have various implementations in mind to add to open source one of them is implemented by megha10 for ZipTextFileReader another can be anySepratorReader like (pipe,comma). If we add enum for every implementation contributor has to do other extra work to enable it rather just adding reader.
Also if user adds his class where he can add it? He has to provide FCQN which comes back to same implementation which I did.

For point 2 I suggest adding shortcut, not removing previous one. Providing FQDN is not easiest way to do so we can make some implementations taking precedence and support it via easiest way.

IMHO providing shortcut method is easiest (or similar to providing enum), and providing enum is good enough, and receiving only FQDN is not convenient, but just has to use. First two things make IDE providing various hints, and the last thing doesn't.

Adding it to enum or even providing shortcut method will be done in 10 lines. I don't think it gives contributors more extra works, but we might need to guide it properly.

The current FileReader implementation is expected to have constructor with a specific signature, which cannot be enforced at compile time. IMO, this should also be fixed along with this patch by introducing some init method in the FileReader interface.`
I am thinking to work on this after this patch we can have init() api which verifies things before it proceeds.
Once we provide init method we are providing user a way to initialize the non Serializable member.

There're various ways to do it.
First of all, your suggestion is valid and I've been using that approach.
One other approach is adding method which receives builder instance and build the instance in open() method. Yes builder instance must be serializable, but if it only has configurations it is not that hard.

[pasalkarsachin1]

@HeartSaVioR

For point 2 I suggest adding shortcut, not removing previous one. Providing FQDN is not easiest way to do so we can make some implementations taking precedence and support it via easiest way.
IMHO providing shortcut method is easiest (or similar to providing enum), and providing enum is good enough, and receiving only FQDN is not convenient, but just has to use. First two things make IDE providing various hints, and the last thing doesn't.
Adding it to enum or even providing shortcut method will be done in 10 lines. I don't think it gives contributors more extra works, but we might need to guide it properly.`

My intention was never to use FQCN it was just to use className rather than Constants. Even you look at my code I am just using class name.

setReader(Class<? extends FileReader> reader)

However what you are suggesting is in HdfsSpout I should add something like below

public hdfsSpout useTextFileReader() {
this.readerType = TextFileReader.class;
return this;
}

public hdfsSpout useSequenceFileReader() {
this.readerType = SequenceFileReader.class;
}

Also have a way to provide way to support FQCN, Correct?

If everyone agrees I will do the change.

[pasalkarsachin1]

@HeartSaVioR @roshannaik @arunmahadevan are you OK with this approach?