You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by krichter722 <gi...@git.apache.org> on 2015/03/09 02:12:47 UTC
[GitHub] storm pull request: avoiding rawtypes in IBold.prepare in order to...
GitHub user krichter722 opened a pull request:
https://github.com/apache/storm/pull/459
avoiding rawtypes in IBold.prepare in order to allow usage without @Supp...
...ressWarnings("rawtypes") which can be quite annoying during intensive usage of storm. Some classes which don't have the generic type of their superclass specified need that specification (it's `Object` for all of them).
Removed some trailing whitespace as well.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/krichter722/storm generics
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/459.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 #459
----
commit 0e6fc05cddf29b36eb84a99967d9faf7cca6f47c
Author: Karl-Philipp Richter <kr...@aol.de>
Date: 2015-03-09T01:09:41Z
avoiding rawtypes in IBold.prepare in order to allow usage without @SuppressWarnings("rawtypes")
----
---
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: avoiding rawtypes in IBold.prepare in order to...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/459#issuecomment-167631143
@knusbaum depending on what @krichter722 is seeking to fix, I could see benefit to cleaning up things like excessive compiler warnings.
@krichter722, what in particular does this fix?
---
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: avoiding rawtypes in IBold.prepare in order to...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/459#issuecomment-148183568
-1
I don't really see the benefit to 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #459: [STORM-2999] Add generics to static type of method paramet...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
I just realized that this PR makes a lot of `@SuppressWarning("rawtypes")` unnecessary. What do you think about adding that as a second commit to this PR?
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
> Is there a Jira issue associated with this pull request? If not, we can make one.
I found https://issues.apache.org/jira/browse/STORM-1048 in issues reported by me which I forgot until now. It either has been fixed before this PR or is definitely fixed with this PR.
---
[GitHub] storm issue #459: [STORM-2999] Add generics to static type of method paramet...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/459
+1
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
> I would either rename that Jira and update the description a bit, or open a new one. Once you've done that, add the jira number into the PR title - look at other Storm PRs for examples.
>
> After that and the checkstyle stuff, I don't see anything else that should prevent this from going in.
@knusbaum Both done.
---
[GitHub] storm pull request: avoiding rawtypes in IBold.prepare in order to...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/459#issuecomment-169141927
@knusbaum are you still a -1 on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #459: avoiding rawtypes in IBold.prepare in order to allo...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/459#discussion_r175226192
--- Diff: external/storm-kafka/pom.xml ---
@@ -57,7 +57,7 @@
<artifactId>maven-checkstyle-plugin</artifactId>
<!--Note - the version would be inherited-->
<configuration>
- <maxAllowedViolations>557</maxAllowedViolations>
+ <maxAllowedViolations>561</maxAllowedViolations>
--- End diff --
@krichter722 curious: what caused us to increase this?
---
[GitHub] storm pull request #459: [STORM-2999] Add generics to static type of method ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/459
---
[GitHub] storm issue #459: [STORM-2999] Add generics to static type of method paramet...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/459
> I just realized that this PR makes a lot of @SuppressWarning("rawtypes") unnecessary. What do you think about adding that as a second commit to this PR?
@krichter722 Sorry for the delay; yes we can hold off if you want to clean up these unnecessary annotations.
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
> It would probably be preferable to do as you suggest, regarding a configuration class, but I'm not sure how much work there is to do there.
I wouldn't do it for all Maps, just the ones carrying complex types (I'm sticking to my description above). On the other hand, having type safe configuration classes for all Maps increases type safety to a maximum, but that'd be a few hundred person hours (I'm saying that after going through a large portion of the code making extremely trivial changes).
> I think it's definitely worth consideration, though. Maybe a dev list email would be a good place to get opinions.
When I find more time, sure. Are you OK with merging this PR? It's a contained unit of work and essentially a pareto optimization that cannot cause any runtime exception that would not already occur and there're no compile time issues.
---
[GitHub] storm issue #459: [STORM-2999] Add generics to static type of method paramet...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
> we can hold off if you want to clean up these unnecessary annotations.
Done.
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/459
@krichter722 I would either rename that Jira and update the description a bit, or open a new one. Once you've done that, add the jira number into the PR title - look at other Storm PRs for examples.
After that and the checkstyle stuff, I don't see anything else that should prevent this from going in.
:+1:
---
[GitHub] storm issue #459: [STORM-2999] Add generics to static type of method paramet...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/459
New changes are to remove warning suppression attributes and look good to me. +1 again.
Thanks @krichter722 for the changes and for your patience. I have merged this to master.
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/459
@krichter722 Yes, these maps are vestiges of the old clojure code. It would probably be preferable to do as you suggest, but I'm not sure how much work there is to do there. IIRC, the config objects are pretty well spread and used throughout the code, and there's some other support code, tests, validation, etc. That may have to be significantly changed. I think it's definitely worth consideration, though. Maybe a dev list email would be a good place to get opinions.
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the issue:
https://github.com/apache/storm/pull/459
@krichter722, I am willing to disregard @knusbaum 's -1, but this PR needs to be updated as most of these files have been moved within the project.
I checked a few of these, and it seems they have already been modified to avoid the compile errors.
Would you take a look?
---
[GitHub] storm pull request #459: avoiding rawtypes in IBold.prepare in order to allo...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on a diff in the pull request:
https://github.com/apache/storm/pull/459#discussion_r175236914
--- Diff: external/storm-kafka/pom.xml ---
@@ -57,7 +57,7 @@
<artifactId>maven-checkstyle-plugin</artifactId>
<!--Note - the version would be inherited-->
<configuration>
- <maxAllowedViolations>557</maxAllowedViolations>
+ <maxAllowedViolations>561</maxAllowedViolations>
--- End diff --
Right, I should have mentioned this on my own. The usage of checkstyle is not clear to me since it enforces extremely strict rules like max. 80 characters line length whereas the code still contains easy to fix issues like variable names. Therefore I assumed that checkstyle has been added without fixing the large amount of complaints and the value 557 has simply been set to the number of complaints. I figured out that it's very hard to diff the violations with and without this PR, however I'm pretty sure that I only added 4 lines longer than 80 characters. I can check again if the 4 more issues besides the existing 557 are an absolute showstopper to merge this PR.
---
[GitHub] storm pull request #459: avoiding rawtypes in IBold.prepare in order to allo...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/459#discussion_r175268334
--- Diff: external/storm-kafka/pom.xml ---
@@ -57,7 +57,7 @@
<artifactId>maven-checkstyle-plugin</artifactId>
<!--Note - the version would be inherited-->
<configuration>
- <maxAllowedViolations>557</maxAllowedViolations>
+ <maxAllowedViolations>561</maxAllowedViolations>
--- End diff --
What we did with checkstyle, for better or worse, was agree on a set of rules and then apply those rules. We didn't go through and fix every checkstyle violation when we did that. However, we agreed that any pull requests should not increase the number of alerts, and preferably decrease them. It would be better if you could reformat any new 80+character lines to please the checkstyle monster.
---
[GitHub] storm pull request: avoiding rawtypes in IBold.prepare in order to...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/459#issuecomment-173776737
@knusbaum
Kindly reminder: are you still a -1 on this?
It may be better to write explicitly that any types can be here so that we can distinguish it is by intention and not missed to write type information.
---
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 #459: [STORM-2999] Add generics to static type of method paramet...
Posted by roshannaik <gi...@git.apache.org>.
Github user roshannaik commented on the issue:
https://github.com/apache/storm/pull/459
@krichter722 thanks for this!
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/459
Yes, I'll retracting my -1. Sorry for letting this one languish.
---
[GitHub] storm issue #459: avoiding rawtypes in IBold.prepare in order to allow usage...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the issue:
https://github.com/apache/storm/pull/459
> I checked a few of these, and it seems they have already been modified to avoid the compile errors.
I seems like `IBolt.prepare`'s `topoConf` has been changed to `Map<String, Object>` everywhere which is a good idea (and a better idea than `<?,?>`). Now that the interface is improved in puncto type safety, the generic types should be used everywhere they're encountered/used anyway, but just not passed down or up the hierarchy. This applies to all topology configuration, metadata and authentication maps since they're using `<String, Object>` at some point in the hierarchy or in a relationship.
I pushed those changes to the generics branch. Since this issue is about `IBolt` I can open a new PR, but it's very similar.
I don't seem to be able to figure out the necessary type for `IScheduler.config` which is sometimes `Map<String, Object>` and `Map<String, Map<String, [Number or Double]>` which is possible in a non-type safe environment, but unnecessarily hard to understand. So, I let it untouched.
There're a lot of usages of the idiom
Map<String, Object> someMap;
Map<String, Object> nestedMap = (Map<String, Object>) someMap.get("someLiteral");
which should imo be encapsulated in a configuration class since the map should contain nothing more complex than wrappers, maybe a single valued collection (list or set), but a map in a map value might cause problems at some point (and probably only does not now because of the excellent test coverage). Also, `"someLiteral"` is the [magic-number anti-pattern](https://en.wikipedia.org/wiki/Magic_number_(programming)#Unnamed_numerical_constants).
> Yes, I'll retracting my -1. Sorry for letting this one languish.
No problem. I'm aware that some of the changes do not introduce improvement immediately because Storm serializes and deserializes a lot, but the changes allow to discover design flaws and to fail earlier in case of a type mismatch.
---
[GitHub] storm pull request: avoiding rawtypes in IBold.prepare in order to...
Posted by krichter722 <gi...@git.apache.org>.
Github user krichter722 commented on the pull request:
https://github.com/apache/storm/pull/459#issuecomment-167631380
> @krichter722, what in particular does this fix?
Compiler warnings.
---
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.
---