[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Thanks for the detailed review and nice suggestions! ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 I think all my comments were addressed, thanks for your patience. LGTM, +1. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo I'd like to get final review and approval from you to check if I miss anything before merging. Could you please help to do this? Thanks! ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 Looks great. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Nice finding. Fixed. Also exported to xml once again. Please check again. Thanks for your patience! [storm-sql-internal-example.xml.txt](https://github.com/apache/storm/files/2198168/storm-sql-internal-example.xml.txt) ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443  [storm-sql-internal-example.xml(1).txt](https://github.com/apache/storm/files/2198080/storm-sql-internal-example.xml.1.txt) They work fine for me. The example diagram had a typo (Fliter). ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo I have exported these diagrams into xml : please download and rename below files to remove `.txt` (github doesn't allow uploading xml file directly) and see they're properly imported in your account of draw.io. [storm-sql-internal-example.xml.txt](https://github.com/apache/storm/files/2198021/storm-sql-internal-example.xml.txt) [storm-sql-internal-workflow.xml.txt](https://github.com/apache/storm/files/2198022/storm-sql-internal-workflow.xml.txt) ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 Raised https://issues.apache.org/jira/browse/STORM-3153 for addressing restoring tests. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 Sounds good. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 Yes test failures look unrelated, one is from server, another one is from cassandra. I'd like to address https://github.com/apache/storm/pull/2443#discussion_r202639091 but OK to address it with new issue, since smaller patch would be easier to merge. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 The test failures look unrelated, not sure why it's suddenly failing. If they don't go away by rebasing to master, we might want to raise issues to track them. I don't know if you're looking at https://github.com/apache/storm/pull/2443#discussion_r202639091, but we can probably make do without the tests for now. If so, it would be good to raise an issue to reintroduce them, the current tests are very minimal. +1 ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 I don't think we need to change it. I just wanted to make a note of it. When this has been merged, we might want to add a couple of notes about debugging to the docs, we can mention the log line there. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Yes, since Streams API doesn't have rich function like executor side initialization for now. We could address it but maybe better to file a new issue and handle it independently. What do you think? ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 Tried out this branch with a storm-sql-kafka example, it seems to work fine. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Rebased. I'm seeing intermittent test failure like below, but not consistent failure. Will try to take a look at once I have time to, but let's move it out of this PR. ``` 17:32:06.845 [SLOT_1027] INFO o.a.s.m.StormMetricRegistry - Starting metrics reporters... 17:32:06.845 [SLOT_1027] INFO o.a.s.s.a.ClientAuthUtils - Got AutoCreds [] 17:32:06.846 [SLOT_1027] INFO o.a.s.d.w.WorkerState - Reading assignments 17:32:06.846 [SLOT_1027] ERROR o.a.s.d.s.Slot - Error when processing event java.io.IOException: java.lang.NullPointerException at org.apache.storm.daemon.supervisor.LocalContainer.launch(LocalContainer.java:54) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.LocalContainerLauncher.launchContainer(LocalContainerLauncher.java:42) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Slot.handleWaitingForBlobUpdate(Slot.java:528) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Slot.stateMachineStep(Slot.java:232) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Slot.run(Slot.java:902) [storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] Caused by: java.lang.NullPointerException at org.apache.storm.daemon.worker.WorkerState.readWorkerExecutors(WorkerState.java:630) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.worker.WorkerState.(WorkerState.java:153) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.worker.Worker.loadWorker(Worker.java:172) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.worker.Worker.lambda$start$39(Worker.java:164) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at java.security.AccessController.doPrivileged(Native Method) ~[?:1.8.0_66] at javax.security.auth.Subject.doAs(Subject.java:422) ~[?:1.8.0_66] at org.apache.storm.daemon.worker.Worker.start(Worker.java:163) ~[storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.LocalContainer.launch(LocalContainer.java:52) ~[storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] ... 4 more 17:32:06.847 [SLOT_1027] ERROR o.a.s.u.Utils - Halting process: Error when processing an event java.lang.RuntimeException: Halting process: Error when processing an event at org.apache.storm.utils.Utils.exitProcess(Utils.java:473) [storm-client-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] at org.apache.storm.daemon.supervisor.Slot.run(Slot.java:949) [storm-server-2.0.0-SNAPSHOT.jar:2.0.0-SNAPSHOT] ``` ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Yeah, thanks for reminding this. Applying the change manually again would be easier than rebasing, since there were nice efforts on storm-sql (checkstyle violation reduction, test config). It would take some efforts to apply the change without messing up current effort. I'll mention you when it's ready. Thanks again! ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2443 @HeartSaVioR Are you still planning on getting this in? I could probably put some time in to trying this out if no one else is willing to test/use storm-sql, so we can get a +1. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 Rebased to apply dropping standalone mode. ---
[GitHub] storm issue #2443: STORM-2406 [Storm SQL] Change underlying API to Streams A...
Github user HeartSaVioR commented on the issue: https://github.com/apache/storm/pull/2443 @srdo Thanks again for the detailed review. Addressed review comments. ---
