You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by revans2 <gi...@git.apache.org> on 2016/11/18 18:58:45 UTC
[GitHub] storm pull request #1786: STORM-1281: LocalCluster, testing4j and testing.cl...
GitHub user revans2 opened a pull request:
https://github.com/apache/storm/pull/1786
STORM-1281: LocalCluster, testing4j and testing.clj to java
This is based off of #1744, but I wanted to make sure that my changes were publicly available.
Now Testing.java replaces testing4j.clj and depends on an improved LocalCluster.java that replaces LocalCluster.clj.
testing.clj now depends on Testing.java and has been moved under storm-clojure.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/revans2/incubator-storm STORM-1281
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1786.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1786
----
commit 55a94f4a32be667a12d68e2775c55add6c0d5f03
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-10-24T20:59:36Z
STORM-1276: line for line translation of nimbus to java
commit 2bb7b1f5aa8338b030e1c03e088b79b8b107866a
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-10-24T21:13:09Z
STORM-1276: missed a TODO in the test code
commit 564ef8308e9ae7efe947cda722ff24ea40f3ad31
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-10-24T21:26:49Z
Removed unused converter functions
commit 6965b56d3d6c1bc9aefd1b7179b50c86e9316a85
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-10-24T22:04:09Z
A bit more refactoring
commit 39148edf07f2028c2edcfecc53ea9bfac525988f
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-11-16T18:25:18Z
Let worker handle empty credentials
commit 2aa9549f9a9ac106f652ca8b6afaaff467cfeee1
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-10-24T20:59:36Z
STORM-1276: line for line translation of nimbus to java
commit da171591580e259c3a264b8889b1940154ffac93
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-11-05T15:33:47Z
STORM-1281: LocalCluster, testing4j and testing.clj to java
commit 083f764306d0d48291110f7e0a0697a6a86c1635
Author: Robert (Bobby) Evans <ev...@yahoo-inc.com>
Date: 2016-11-18T13:18:00Z
Fix some race conditions in nimbus and make test plan compiler share a local cluster (runs faster)
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1786
@ptgoetz I added in javadocs for most of the 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1786
The test failures look unrelated. They seem to be the same windowing integration tests failing again.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1786: STORM-1281: LocalCluster, testing4j and testing.cl...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on a diff in the pull request:
https://github.com/apache/storm/pull/1786#discussion_r90311211
--- Diff: storm-core/src/jvm/org/apache/storm/testing/CompletableSpout.java ---
@@ -0,0 +1,39 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.storm.testing;
+
+public interface CompletableSpout {
--- End diff --
Didn't mean to imply that the class should be moved now as part of this pull request, but rather that it's something we should consider doing later.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1786: STORM-1281: LocalCluster, testing4j and testing.cl...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1786#discussion_r88749944
--- Diff: storm-core/test/clj/org/apache/storm/clojure_test.clj ---
@@ -1,159 +0,0 @@
-;; Licensed to the Apache Software Foundation (ASF) under one
--- End diff --
There is a copy of this in storm-clojure already so no need to two of them.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/1786
@revans2 Thanks. Still +1.
(One procedural nit: Rebasing/squashing on big PRs like this can make reviews a little harder. E.g. After you added the javadocs I wanted to review just that change, but the squash forced me to wade through all the other changes to find the ones I was looking for. Not a big deal, but something to keep in mind.)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by ptgoetz <gi...@git.apache.org>.
Github user ptgoetz commented on the issue:
https://github.com/apache/storm/pull/1786
+1. I haven't done any manual testing of this patch, but I reviewed the code and it looks good.
I wouldn't mind seeing more javadoc comments in some of the new classes/interfaces, but it can always be added later, so I don't see that as a merge blocker or something that needs to be addressed right away.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1786
Rebased...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1786
Rebased now that java nimbus is in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1786: STORM-1281: LocalCluster, testing4j and testing.cl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1786
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1786: STORM-1281: LocalCluster, testing4j and testing.clj to ja...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1786
@ptgoetz I'll look at adding in some javadocs. In general I tried to add javadocs where there were clojure doc strings, but there were not too many of them either.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---