[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1781 +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user mkoch1 commented on the issue: https://github.com/apache/storm/pull/1781 Squashed. I don't really see how this should cause any conflicts though. The only changes in existing code are a couple of namespace corrections (to avoid wildcard imports, as suggested earlier) and documentation. Other than that, it's all new code in new classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1781 @mkoch1 Could you squash commits into one if you don't mind? I know it's more than 2000 lines change but looks like separating commits are not needed. And I would like to wait for 1.1.0 RC vote to pass to avoid any conflicts with the release. That says, this is not going to be included to 1.1.0. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user mkoch1 commented on the issue: https://github.com/apache/storm/pull/1781 Rebase is done, sorry for the delay. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/1781 @mkoch1 Could you rebase this? After rebasing I'll merge this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1781 @mkoch1 Thanks for addressing the license issue. The way you handled the borrowed code looks fine, the important part is to leave the original copyright statement, which you did. Thanks for pointing that out. +1 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user mkoch1 commented on the issue: https://github.com/apache/storm/pull/1781 Did not notice that, sorry. Replaced now with an external resource implementation. The code is not all that complex so it's not a big deal. One thing I should point out: I borrowed the base implementation from a different project, also under Apache 2.0 license. I've included their copyright line in the file: https://github.com/apache/storm/pull/1781/commits/e46496337af84a610ef742c8cf5988d8dab19e47#diff-a8ce0c2f5e3ce0193d3ebc49f54301fa Original code: https://github.com/spring-projects/spring-insight-plugins/blob/master/collection-plugins/cassandra12/src/test/java/com/springsource/insight/plugin/cassandra/embeded/EmbeddedCassandraService.java Please let me know if there are any issues with this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user ptgoetz commented on the issue: https://github.com/apache/storm/pull/1781 Unfortunately this patch uses Cassandra-unit, which is LGPL licensed, so in it's current state we can't accept it. For that reason I am -1. We've run into this before, and I've lobbied the Cassandra-unit author to switch to an Apache compatible license (https://github.com/jsevellec/cassandra-unit/issues/163) to no avail. We can, however, accept it if the Cassandra-unit dependency and tests that depend on it are removed. Again, it's unfortunate. If more people encourage the Cassandra-unit author to choose a different license, perhaps he'll consider it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user mkoch1 commented on the issue: https://github.com/apache/storm/pull/1781 That should do it - the batching option has been removed from MapState, in favor of parallel processing with existing opaque and transactional logic to handle consistency. Cassandra batch statements are more trouble than they're worth. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #1781: STORM-1369: Add MapState implementation to storm-cassandr...
Github user mkoch1 commented on the issue: https://github.com/apache/storm/pull/1781 Pls note that mutliPut needs an update - will push commit today or tomorrow. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
