The change on storm-redis seems to require breaking backward compatibility, so I would love to see another pull request to integrate it via general review process. If it can be integrated to SQE without changing storm-redis, that would be nice.
Does it make sense? - Jungtaek Lim (HeartSaVioR) 2016년 9월 29일 (목) 오전 6:08, P. Taylor Goetz <ptgo...@gmail.com>님이 작성: > The changes are available in GitHub, I just overlooked it. And they’re > actually neatly contained in two commits: > > The changes to storm-kafka can be found here: > > > https://github.com/jwplayer/storm/commit/2069c76695a225e4bb8f402c89e572836104755a > > > The changes to storm-redis are here: > > > https://github.com/jwplayer/storm/commit/30d000d3ff673efa8b927d23e554a705fb2928b8 > <https://github.com/jwplayer/storm/commit/2069c76695a225e4bb8f402c89e572836104755a> > > > The changes to storm-kafka are straightforward and implemented in such a > way that they would be useful for use cases outside of SQE. As the commit > message states, it adds a new kafka deserialization scheme (FullScheme) > that includes the key, value, topic, partition and offset when reading from > kafka, which is a feature I can see as being valuable for some use cases. I > would be +1 for merging that code. > > The changes to storm-redis are a little different, as Morrigan pointed > out, because it only addresses the Trident API, but IMHO it looks like a > good direction. > > @HeartSavior — Would you have some time to take a look at the storm-redis > changes and provide your opinion, since you’re one of the original authors > of that code? > > -Taylor > > > On Sep 26, 2016, at 6:28 PM, Jungtaek Lim <kabh...@gmail.com> wrote: > > Great! > > For storm-redis, we might need to modify key/value mapper to use byte[] > rather than String. > When I co-authored storm-redis, I forgot considering binary format of > serde. If we want to address that part, we can also address it. > > 2016년 9월 27일 (화) 오전 7:19, Morrigan Jones <morri...@jwplayer.com>님이 작성: > > Sure, when I can. Storm-kafka should be pretty easy. The storm-redis one > will require more work to make it more complete. > > On Mon, Sep 26, 2016 at 6:09 PM, P. Taylor Goetz <ptgo...@gmail.com> > wrote: > > Thanks for the explanation Morrigan! > > Would you be willing to provide a pull request or patch so the community > can review? > > It sounds like at least some of the changes you mention could be useful > > to > > the broader community (beyond the SQL effort). > > Thanks again, > > -Taylor > > On Sep 26, 2016, at 4:40 PM, Morrigan Jones <morri...@jwplayer.com> > > wrote: > > > storm-kafka - This is needed because storm-kafka does not provide a > > scheme > > class that gives you the key, value (payload), partition, and offset. > MessageMetadataScheme.java comes comes closest, but is missing the key. > This was a pretty simple change on my part. > > storm-redis - This is needed for proper support of Redis hashes. The > existing storm-redis uses a static string (additionalKey in > the RedisDataTypeDescription class) for the field name in hash types. I > updated it to use a configurable KeyFactory for both the hash name and > > the > > field name. We also added some limited support for set types. This is > admittedly the messiest between the two jars since we only cared about > > the > > trident states and would require a lot more changes to get storm-redis > > more > > "feature complete" overall. > > > > > On Mon, Sep 26, 2016 at 4:03 PM, P. Taylor Goetz <ptgo...@gmail.com> > > wrote: > > > Sounds good. I’ll find out if it builds against 2.x. If so I’ll go > > that > > direction. Otherwise I’ll come back with my findings and we can > > discuss > > it > > further. > > I notice there are jars in the git repo that we obviously can’t > > import. > > They look like they might be custom JWPlayer builds of storm-kafka and > storm-redis. > > Morrigan — Do you know if there is any differences there that required > custom builds of those components? > > -Taylor > > On Sep 26, 2016, at 3:31 PM, Bobby Evans > > <ev...@yahoo-inc.com.INVALID > > > wrote: > > Does it compile against 2.X? If so I would prefer to have it go > > there, > > and then possibly 1.x if people what it there too. - Bobby > > > On Monday, September 26, 2016 12:47 PM, P. Taylor Goetz < > > ptgo...@gmail.com> wrote: > > > The IP Clearance vote has passed and we are now able to import the > > SQE > > code. > > > The question now is to where do we want to import the code? > > My inclination is to import it to “external” in the 1.x branch. It > > can > > be ported to other branches as necessary/if desired. An alternative > > would > > be to treat it as a feature branch, but I’d rather take the former > > approach. > > > Thought/opinions? > > -Taylor > > On Sep 21, 2016, at 8:39 PM, P. Taylor Goetz <ptgo...@gmail.com> > > wrote: > > > My apologies. I meant to cc dev@, but didn't. Will forward in a > > bit... > > > The vote (lazy consensus) is underway on general@incubator, and > > will > > close in less than 72 hours. After that the code can be merged. > > > -Taylor > > On Sep 21, 2016, at 7:02 PM, Jungtaek Lim <kabh...@gmail.com> > > wrote: > > > Hi dev, > > While code contribution of SQE is in progress, I would like to > > continue > > discussion how to merge SQE and Storm SQL. > > I did an analysis of merging SQE and Storm SQL in both side, > > integrating > > SQE to Storm SQL and vice versa. > https://cwiki.apache.org/confluence/display/STORM/ > > Technical+analysis+of+merging+SQE+and+Storm+SQL > > > As I commented to that page, since I'm working on Storm SQL for > > some > > weeks > > I can be (heavily) biased. So I'd really appreciated if someone can > > do > > another analysis. > > Please feel free to share your thought about this analysis, or > > another > > proposal if you have any, or other things about the merging. > > Thanks, > Jungtaek Lim (HeartSaVioR) > > > > -- > Morrigan Jones > Principal Engineer > *JW*PLAYER | Your Way to Play > morri...@jwplayer.com | jwplayer.com > > > > > > -- > Morrigan Jones > Principal Engineer > *JW*PLAYER | Your Way to Play > morri...@jwplayer.com | jwplayer.com > > >
