You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@eagle.apache.org by asdf2014 <gi...@git.apache.org> on 2017/04/19 11:45:45 UTC
[GitHub] eagle pull request #925: [EAGLE-1012] Some language level problems
GitHub user asdf2014 opened a pull request:
https://github.com/apache/eagle/pull/925
[EAGLE-1012] Some language level problems
Some language level problems
* Spell
* Duplicated
* Lambda
* Collection
* String
* instanceof
* Complex Method
* Exception
* AutoClose
* Synchronized
Details see [attachment files](https://issues.apache.org/jira/secure/attachment/12863947/Eagle%20Language%20Level%20Problems.txt).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/asdf2014/eagle language_level_problem
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/eagle/pull/925.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 #925
----
commit f935407f672b92146f57ad2da5ef7c36dc2931a5
Author: asdf2014 <15...@qq.com>
Date: 2017-04-19T11:40:16Z
Fix spell issues
----
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by jhsenjaliya <gi...@git.apache.org>.
Github user jhsenjaliya commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114253159
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertPublisherImpl.java ---
@@ -199,7 +194,7 @@ private void closePlugins(List<AlertPublishPlugin> toBeClosed) {
try {
p.close();
} catch (Exception e) {
- LOG.error(String.format("Error when close publish plugin {}!", p.getClass().getCanonicalName()), e);
+ LOG.error(String.format("Error when close publish plugin %s!", p.getClass().getCanonicalName()), e);
--- End diff --
people are using this interchangeably
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by jhsenjaliya <gi...@git.apache.org>.
Github user jhsenjaliya commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114253102
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertEagleStorePlugin.java ---
@@ -62,10 +60,9 @@ public void onAlert(AlertStreamEvent event) throws Exception {
if (eventList == null || eventList.isEmpty()) {
return;
}
- List<AlertPublishEvent> alertEvents = new ArrayList<>();
- for (AlertStreamEvent e : eventList) {
- alertEvents.add(AlertPublishEvent.createAlertPublishEvent(e));
- }
+ List<AlertPublishEvent> alertEvents = eventList.stream().map(
--- End diff --
i feel its fancy to use newer java features, but old syntax is more readable, may be its just a coding preference
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114255903
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertEagleStorePlugin.java ---
@@ -62,10 +60,9 @@ public void onAlert(AlertStreamEvent event) throws Exception {
if (eventList == null || eventList.isEmpty()) {
return;
}
- List<AlertPublishEvent> alertEvents = new ArrayList<>();
- for (AlertStreamEvent e : eventList) {
- alertEvents.add(AlertPublishEvent.createAlertPublishEvent(e));
- }
+ List<AlertPublishEvent> alertEvents = eventList.stream().map(
--- End diff --
Maybe you are right, i will roll it back. :+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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by jhsenjaliya <gi...@git.apache.org>.
Github user jhsenjaliya commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114252915
--- Diff: eagle-core/eagle-alert-parent/eagle-alert-service/src/main/java/org/apache/eagle/service/alert/resolver/AttributeResolverFactory.java ---
@@ -43,8 +43,6 @@ public static AttributeResolvable getAttributeResolver(String fieldResolverName,
fieldResolvableCache.put(fieldResolverName, instance);
} catch (ClassNotFoundException e) {
throw new AttributeResolveException("Attribute Resolver in type of " + fieldResolverName + " is not found", e);
- } catch (InstantiationException | IllegalAccessException e) {
--- End diff --
instead of removing this, it can be used to describe the error, IDE is suggesting to collapse the catch block may be because it has identical error msg (throw new AttributeResolveException(e)).
---
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] eagle issue #925: [EAGLE-1012] Some language level problems in `eagle-core` ...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on the issue:
https://github.com/apache/eagle/pull/925
@jhsenjaliya You are welcome. Sorry, i just note it. Let’s finish this after `v0.5.0` together.
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114254151
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertEagleStorePlugin.java ---
@@ -62,10 +60,9 @@ public void onAlert(AlertStreamEvent event) throws Exception {
if (eventList == null || eventList.isEmpty()) {
return;
}
- List<AlertPublishEvent> alertEvents = new ArrayList<>();
- for (AlertStreamEvent e : eventList) {
- alertEvents.add(AlertPublishEvent.createAlertPublishEvent(e));
- }
+ List<AlertPublishEvent> alertEvents = eventList.stream().map(
--- End diff --
I think it will bring better maintainability and [performance](http://blog.takipi.com/benchmark-how-java-8-lambdas-and-streams-can-make-your-code-5-times-slower/).
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114254748
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertPublisherImpl.java ---
@@ -199,7 +194,7 @@ private void closePlugins(List<AlertPublishPlugin> toBeClosed) {
try {
p.close();
} catch (Exception e) {
- LOG.error(String.format("Error when close publish plugin {}!", p.getClass().getCanonicalName()), e);
+ LOG.error(String.format("Error when close publish plugin %s!", p.getClass().getCanonicalName()), e);
--- End diff --
`String.format` should use `%s` rather than `{}`
```java
@Test
public void testFormat() {
assertEquals("a{}c", String.format("a{}c", "b"));
assertEquals("abc", String.format("a%sc", "b"));
}
```
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 closed the pull request at:
https://github.com/apache/eagle/pull/925
---
[GitHub] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114253745
--- Diff: eagle-core/eagle-alert-parent/eagle-alert-service/src/main/java/org/apache/eagle/service/alert/resolver/AttributeResolverFactory.java ---
@@ -43,8 +43,6 @@ public static AttributeResolvable getAttributeResolver(String fieldResolverName,
fieldResolvableCache.put(fieldResolverName, instance);
} catch (ClassNotFoundException e) {
throw new AttributeResolveException("Attribute Resolver in type of " + fieldResolverName + " is not found", e);
- } catch (InstantiationException | IllegalAccessException e) {
--- End diff --
I see. I will roll it back.
---
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] eagle issue #925: [EAGLE-1012] Some language level problems in `eagle-core` ...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on the issue:
https://github.com/apache/eagle/pull/925
@jhsenjaliya Okay, i see. Thx for your `code review`.
---
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] eagle issue #925: [EAGLE-1012] Some language level problems in `eagle-core` ...
Posted by jhsenjaliya <gi...@git.apache.org>.
Github user jhsenjaliya commented on the issue:
https://github.com/apache/eagle/pull/925
@asdf2014 Thanks again, lets work on this after 0.5
---
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] eagle issue #925: [EAGLE-1012] Some language level problems in `eagle-core` ...
Posted by jhsenjaliya <gi...@git.apache.org>.
Github user jhsenjaliya commented on the issue:
https://github.com/apache/eagle/pull/925
@asdf2014 , i havent gone through all the changes yet, but looks like these are what some particular IDE would recommend, some are real improvements as well. lets check this out after 0.5, Thanks for the efforts !
---
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] eagle pull request #925: [EAGLE-1012] Some language level problems in `eagle...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on a diff in the pull request:
https://github.com/apache/eagle/pull/925#discussion_r114278551
--- Diff: eagle-core/eagle-alert-parent/eagle-alert/alert-engine/src/main/java/org/apache/eagle/alert/engine/publisher/impl/AlertEagleStorePlugin.java ---
@@ -62,10 +60,9 @@ public void onAlert(AlertStreamEvent event) throws Exception {
if (eventList == null || eventList.isEmpty()) {
return;
}
- List<AlertPublishEvent> alertEvents = new ArrayList<>();
- for (AlertStreamEvent e : eventList) {
- alertEvents.add(AlertPublishEvent.createAlertPublishEvent(e));
- }
+ List<AlertPublishEvent> alertEvents = eventList.stream().map(
--- End diff --
I have created a non-professional benchmark [test](https://github.com/asdf2014/yuzhouwan/blob/master/yuzhouwan-hacker/src/main/java/com/yuzhouwan/hacker/lambda/StreamPerformance.java#L19). After adding `-Xmx2G -Xms2G -Xmn1G -XX:+AlwaysPreTouch` params for reducing the impact from jvm resources, then found the performance gap is not very large, and the performance of `lambda stream` is better than `simple for loop` after the data becomes larger.
---
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] eagle issue #925: [EAGLE-1012] Some language level problems in `eagle-core` ...
Posted by asdf2014 <gi...@git.apache.org>.
Github user asdf2014 commented on the issue:
https://github.com/apache/eagle/pull/925
If these improvements are necessary, we should step by step merge to avoid too much code conflict. So, creating multiple sub `PRs` for `EAGLE-1012` will be more suitable for this situation. What do you think? @jhsenjaliya @qingwen220
---
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.
---