You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@rya.apache.org by jdasch <gi...@git.apache.org> on 2017/10/26 16:32:13 UTC
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
GitHub user jdasch opened a pull request:
https://github.com/apache/incubator-rya/pull/248
RYA-356 Added a Twill App for running the periodic service
<!--
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.
-->
## Description
>What Changed?
[Brief Description of what changed]
### Tests
>Coverage?
[Description of what tests were written]
### Links
[Jira](https://issues.apache.org/jira/browse/RYA-NUMBER)
### Checklist
- [ ] Code Review
- [ ] Squash Commits
#### People To Reivew
@amihalik
@meiercaleb
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jdasch/incubator-rya RYA-356
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rya/pull/248.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 #248
----
commit cb6001d7fe515178742b8233368850fe969b48f0
Author: jdasch <hc...@gmail.com>
Date: 2017-09-07T20:57:13Z
RYA-356 Added a Twill App for running the periodic service
----
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by amihalik <gi...@git.apache.org>.
Github user amihalik commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148799979
--- Diff: extras/periodic.notification/api/src/main/java/org/apache/rya/periodic/notification/api/PeriodicNotificationClient.java ---
@@ -36,29 +36,26 @@
* Adds a new notification to be registered with the {@link NotificationCoordinatorExecutor}
* @param notification - notification to be added
*/
- public void addNotification(PeriodicNotification notification);
--- End diff --
tldr: I think we should pull the PR as is, but don't perform "style changes" and "feature changes" in a single PR in the future.
I didn't know that the modifier was redundant. I'd keep in the "public" modifiers to be consistent with the code base, but I'm fine with the change since it is consistent within the sub module.
Overall, I think there are two key things to keep in mind:
1. Style changes may turn off reviewers from looking at your "feature change" PR. Style changes typically make your code commit a lot larger than necessary and muddy important "feature changes" with a lot of style changes.
2. Defining a community style and tools to evaluate and enforce this style is very important. Consistent style across the code base will lower the barrier of entry for new developers, will build a larger community, etc. I would enjoy reviewing and pulling those changes in. But I'd be equally turned off if those style changes included "feature changes".
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148121063
--- Diff: extras/rya.pcj.fluo/pcj.fluo.integration/src/test/java/org/apache/rya/indexing/pcj/fluo/integration/KafkaExportIT.java ---
@@ -249,10 +246,10 @@ public void average() throws Exception {
// Create the PCJ in Fluo and load the statements into Rya.
final String pcjId = loadDataAndCreateQuery(sparql, statements);
-
- try(FluoClient fluo = new FluoClientImpl(super.getFluoConfiguration())) {
- FluoITHelper.printFluoTable(fluo);
- }
+
+// try(FluoClient fluo = new FluoClientImpl(super.getFluoConfiguration())) {
--- End diff --
Delete.
---
[GitHub] incubator-rya issue #248: RYA-356 Added a Twill App for running the periodic...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-rya/pull/248
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-rya-master-with-optionals-pull-requests/548/
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148546455
--- Diff: extras/periodic.notification/service/src/main/java/org/apache/rya/periodic/notification/application/PeriodicNotificationApplicationConfiguration.java ---
@@ -85,170 +85,170 @@ public PeriodicNotificationApplicationConfiguration(Properties props) {
setPrunerThreads(Integer.parseInt(props.getProperty(PRUNER_THREADS, "1")));
setCoordinatorThreads(Integer.parseInt(props.getProperty(COORDINATOR_THREADS, "1")));
}
-
+
/**
* Sets the name of the Fluo Application
- * @param fluoAppName
+ * @param fluoAppName
*/
- public void setFluoAppName(String fluoAppName) {
- set(FLUO_APP_NAME, Preconditions.checkNotNull(fluoAppName));
+ public void setFluoAppName(final String fluoAppName) {
+ set(FLUO_APP_NAME, Objects.requireNonNull(fluoAppName));
--- End diff --
I see what you're saying (bonus points for finding char length irony!), but the semantics of the public modifier are fixed and known. The method qualifier could be anything, that's why I prefer to leave it in. Again, it is just a preference and I'm happy to switch to a static import if this is a sticking point.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148272970
--- Diff: extras/periodic.notification/api/src/main/java/org/apache/rya/periodic/notification/api/PeriodicNotificationClient.java ---
@@ -36,29 +36,26 @@
* Adds a new notification to be registered with the {@link NotificationCoordinatorExecutor}
* @param notification - notification to be added
*/
- public void addNotification(PeriodicNotification notification);
--- End diff --
Per the [JLS](https://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.4): "It is permitted, but discouraged as a matter of style, to redundantly specify the public and/or abstract modifier for a method declared in an interface." This is consistent with how Java and Guava style their code. I realize that other Rya interfaces may have these redundant modifiers, but I was just correcting this instance because I was already modifying that file.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148363484
--- Diff: extras/periodic.notification/service/src/main/java/org/apache/rya/periodic/notification/application/PeriodicNotificationApplicationConfiguration.java ---
@@ -85,170 +85,170 @@ public PeriodicNotificationApplicationConfiguration(Properties props) {
setPrunerThreads(Integer.parseInt(props.getProperty(PRUNER_THREADS, "1")));
setCoordinatorThreads(Integer.parseInt(props.getProperty(COORDINATOR_THREADS, "1")));
}
-
+
/**
* Sets the name of the Fluo Application
- * @param fluoAppName
+ * @param fluoAppName
*/
- public void setFluoAppName(String fluoAppName) {
- set(FLUO_APP_NAME, Preconditions.checkNotNull(fluoAppName));
+ public void setFluoAppName(final String fluoAppName) {
+ set(FLUO_APP_NAME, Objects.requireNonNull(fluoAppName));
--- End diff --
I think it's funny that you are advocating leaving out the public modifier for the your interface methods to eliminate redundancy, yet here you think that the redundancy is okay. Ironically, the public modifier only costs you 7 chars as well, and I think you get an equal amount of readility/info out of that.
---
[GitHub] incubator-rya issue #248: RYA-356 Added a Twill App for running the periodic...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/incubator-rya/pull/248
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-rya-master-with-optionals-pull-requests/542/
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148105230
--- Diff: extras/indexing/src/main/java/org/apache/rya/indexing/pcj/matching/AccumuloIndexSetProvider.java ---
@@ -155,9 +152,9 @@ public int size() throws Exception {
*/
private List<ExternalTupleSet> getAccIndices() throws Exception {
- requireNonNull(conf);
- final String tablePrefix = requireNonNull(conf.get(RdfCloudTripleStoreConfiguration.CONF_TBL_PREFIX));
- final Connector conn = requireNonNull(ConfigUtils.getConnector(conf));
+ Objects.requireNonNull(conf);
--- End diff --
What was wrong with the static method import?
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148120928
--- Diff: extras/rya.pcj.fluo/pcj.fluo.integration/src/test/java/org/apache/rya/indexing/pcj/fluo/integration/KafkaExportIT.java ---
@@ -92,8 +89,8 @@ public void newResultsExportedTest() throws Exception {
// Create the PCJ in Fluo and load the statements into Rya.
final String pcjId = loadDataAndCreateQuery(sparql, statements);
- FluoITHelper.printFluoTable(super.getFluoConfiguration());
-
+ //FluoITHelper.printFluoTable(super.getFluoConfiguration());
--- End diff --
Delete these. Don't comment out.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148271382
--- Diff: extras/rya.pcj.fluo/pcj.fluo.integration/src/test/java/org/apache/rya/indexing/pcj/fluo/integration/KafkaExportIT.java ---
@@ -249,10 +246,10 @@ public void average() throws Exception {
// Create the PCJ in Fluo and load the statements into Rya.
final String pcjId = loadDataAndCreateQuery(sparql, statements);
-
- try(FluoClient fluo = new FluoClientImpl(super.getFluoConfiguration())) {
- FluoITHelper.printFluoTable(fluo);
- }
+
+// try(FluoClient fluo = new FluoClientImpl(super.getFluoConfiguration())) {
--- End diff --
Done.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148361952
--- Diff: extras/periodic.notification/api/src/main/java/org/apache/rya/periodic/notification/api/PeriodicNotificationClient.java ---
@@ -36,29 +36,26 @@
* Adds a new notification to be registered with the {@link NotificationCoordinatorExecutor}
* @param notification - notification to be added
*/
- public void addNotification(PeriodicNotification notification);
--- End diff --
Yeah, as a matter of style it's fine, but I'd prefer that you stay consistent with the code base. Perhaps @amihalik could weigh in on this.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148115111
--- Diff: extras/rya.benchmark/src/main/config/common.options ---
@@ -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.
+#
+
--- End diff --
Add a comment indicating that these are meant to be example values.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148271883
--- Diff: extras/periodic.notification/service/src/main/java/org/apache/rya/periodic/notification/application/PeriodicNotificationApplicationConfiguration.java ---
@@ -85,170 +85,170 @@ public PeriodicNotificationApplicationConfiguration(Properties props) {
setPrunerThreads(Integer.parseInt(props.getProperty(PRUNER_THREADS, "1")));
setCoordinatorThreads(Integer.parseInt(props.getProperty(COORDINATOR_THREADS, "1")));
}
-
+
/**
* Sets the name of the Fluo Application
- * @param fluoAppName
+ * @param fluoAppName
*/
- public void setFluoAppName(String fluoAppName) {
- set(FLUO_APP_NAME, Preconditions.checkNotNull(fluoAppName));
+ public void setFluoAppName(final String fluoAppName) {
+ set(FLUO_APP_NAME, Objects.requireNonNull(fluoAppName));
--- End diff --
I generally prefer to leave the class name in. For only 7 characters you get reduced ambiguity and increased readability.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148272083
--- Diff: extras/indexing/src/main/java/org/apache/rya/indexing/pcj/matching/AccumuloIndexSetProvider.java ---
@@ -155,9 +152,9 @@ public int size() throws Exception {
*/
private List<ExternalTupleSet> getAccIndices() throws Exception {
- requireNonNull(conf);
- final String tablePrefix = requireNonNull(conf.get(RdfCloudTripleStoreConfiguration.CONF_TBL_PREFIX));
- final Connector conn = requireNonNull(ConfigUtils.getConnector(conf));
+ Objects.requireNonNull(conf);
--- End diff --
Nothing really, but I prefer it leave it in for readability. Same as comment below.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148108793
--- Diff: extras/periodic.notification/service/src/main/java/org/apache/rya/periodic/notification/application/PeriodicNotificationApplicationConfiguration.java ---
@@ -85,170 +85,170 @@ public PeriodicNotificationApplicationConfiguration(Properties props) {
setPrunerThreads(Integer.parseInt(props.getProperty(PRUNER_THREADS, "1")));
setCoordinatorThreads(Integer.parseInt(props.getProperty(COORDINATOR_THREADS, "1")));
}
-
+
/**
* Sets the name of the Fluo Application
- * @param fluoAppName
+ * @param fluoAppName
*/
- public void setFluoAppName(String fluoAppName) {
- set(FLUO_APP_NAME, Preconditions.checkNotNull(fluoAppName));
+ public void setFluoAppName(final String fluoAppName) {
+ set(FLUO_APP_NAME, Objects.requireNonNull(fluoAppName));
--- End diff --
Static import might look a little cleaner here.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148271436
--- Diff: extras/rya.benchmark/src/main/config/common.options ---
@@ -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.
+#
+
--- End diff --
Done.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by meiercaleb <gi...@git.apache.org>.
Github user meiercaleb commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148107974
--- Diff: extras/periodic.notification/api/src/main/java/org/apache/rya/periodic/notification/api/PeriodicNotificationClient.java ---
@@ -36,29 +36,26 @@
* Adds a new notification to be registered with the {@link NotificationCoordinatorExecutor}
* @param notification - notification to be added
*/
- public void addNotification(PeriodicNotification notification);
--- End diff --
Not thrilled about removing public from the interface method headers. I understand that it is redundant, but I think that most style guides suggest that you include the public declaration. Also, I think that pretty much all interfaces within Rya explicitly declare their methods as public.
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-rya/pull/248
---
[GitHub] incubator-rya pull request #248: RYA-356 Added a Twill App for running the p...
Posted by jdasch <gi...@git.apache.org>.
Github user jdasch commented on a diff in the pull request:
https://github.com/apache/incubator-rya/pull/248#discussion_r148271410
--- Diff: extras/rya.pcj.fluo/pcj.fluo.integration/src/test/java/org/apache/rya/indexing/pcj/fluo/integration/KafkaExportIT.java ---
@@ -92,8 +89,8 @@ public void newResultsExportedTest() throws Exception {
// Create the PCJ in Fluo and load the statements into Rya.
final String pcjId = loadDataAndCreateQuery(sparql, statements);
- FluoITHelper.printFluoTable(super.getFluoConfiguration());
-
+ //FluoITHelper.printFluoTable(super.getFluoConfiguration());
--- End diff --
Done.
---