STORM-828 HdfsBolt takes a lot of configuration, need good defaults

[redsanket]

Removing configs from HDFSFileTopology example. I have made neccessary config and constructor changes as per my understanding. It will be nice to know if there is anything I need to look at.

[dossett]

Are there any guarantees that this bolt actually writes out comma separated values? I don't see that, which makes the class name somewhat misleading.

Similar observation about the TSV bolt further down.

[redsanket]

I guess I could set the RecordDefaultDelimiter to ",". Yes, thanks for pointing it out, I will make the changes and it up soon

[dossett]

I think it would take much more work than that and a clearer definition of what the CSVFileBolt does, IMO. What should the user expect from the behavior of CSVBolt.emit() ? Will it take arbitrary values from a tuple and join them together with commas? Will it escape characters? etc.

Another approach would be to create a separate bolt that does nothing but generate CSV output and then passes the results to any other bolt the user wanted. I often create pre-processing bolts like that.

[redsanket]

For futher clarification, I was further thinking over this issue and I presume currently the spout emits tuples in the form of fields and the csv or tsv bolt joins them with the specified record format delimiter. If it is a ",", all tuples will be appended by a comma. The HdfsBolt actually does this implementation when the execute is called upon it. The TSV or CSV are just abstractions to get the intended values based on the record delimiter. Can you please let me know what exactly has to be done with an example, I thought I got your point but right now I cannot clearly picture it. It will be great if you can get back with a reply. Thanks a lot.

[revans2]

If CSV and TSV formats are a bit controversial perhaps we can file a separate JIRA to implement them properly, with escaping, etc. and remove them from here.

[harshach]

@redsanket why we need CSVBolt and TSVBolt when there is DelimitedRecordFormat which users can configure

[redsanket]

ok that seems to be true, I will change that. Mostly it will be down to refactoring a piece of code, we might not have to make it more specific then. Thanks

[harshach]

@redsanket I am not sure why this patch needs to change all these files. Why can't we just add default values to the variables here https://github.com/apache/storm/blob/master/external/storm-hdfs/src/main/java/org/apache/storm/hdfs/bolt/AbstractHdfsBolt.java#L53 similar to what we have in hive connector https://github.com/apache/storm/blob/master/external/storm-hive/src/main/java/org/apache/storm/hive/common/HiveOptions.java#L33

[redsanket]

Yes I have made the changes, I will update my pull request soon

[revans2]

Extra blank line.

[revans2]

Sorry it took me so long to review this. For the most part things look good. I made a few comments on the HdfBolt that I would like to see reflected in the SequenceFileBolt too. Also an upmerge would be good.

Thank you for being so patient with us.

[dossett]

@redsanket Are you still working on this?