STORM-2477: Add generics to Config types #2072
Conversation
There was a problem hiding this comment.
WorkerState.getTopologyConf and Executor.normalizedComponentConf could also be updated to return Map<String, Object>.
Other than those minor nitpicks, this seems good. The project still builds for me after running the script, so +1 (didn't run tests, they are failing for me on master for some reason)
edit: Nevermind the test failures, just needed to run thrift.
| Assert.assertNull(AuthUtils.GetConfiguration(emptyMap)); | ||
| } | ||
|
|
||
| @Test | ||
| public void getAutoCredentialsTest() { | ||
| Map emptyMap = new HashMap<String, String>(); | ||
| Map<String, Collection<String>> map = new HashMap<String, Collection<String>>(); | ||
| Map<String, Object> emptyMap = new HashMap<>(); |
There was a problem hiding this comment.
Nit: Consider replacing this with Collections.emptyMap() in L107.
| this.collector = collector; | ||
| final Configuration hbConfig = HBaseConfiguration.create(); | ||
|
|
||
| Map<String, Object> conf = (Map<String, Object>)map.get(this.configKey); | ||
| Map<String, Object> conf = (Map<String, Object>)topoConf.get(this.configKey); |
There was a problem hiding this comment.
Won't this cast become redundant when transform.sh has been run?
There was a problem hiding this comment.
No because of the order of operations. It is casting the value returned by topoConf.get, not topoConf itself.
There was a problem hiding this comment.
Right, read a bit too fast there.
|
@srdo I addressed your review comments about empty map. |
|
@revans2 I meant you could get rid of the emptyMap variables entirely. |
|
@srdo can do. Wow you really don't like ugly code :) |
|
@srdo done |
|
@revans2 Sorry, didn't mean to nitpick you to death :) +1 |
|
@srdo please continue to nit pick. I like good clean code and I am happy to see people call me out on things that should be fixed. I find that people who read the code closely and nit pick find more serious bugs during the review which saves me a lot of time debugging and fixing them later on. |
revans2
force-pushed
the
STORM-2477
branch
2 times, most recently
from
580ac0f
to
65573cd
Compare
|
@ptgoetz @HeartSaVioR if either of you could take a look at this I would appreciate it. |
| @@ -27,6 +27,7 @@ before_install: | |||
| - rvm use 2.1.5 --install | |||
| - nvm install 0.12.2 | |||
| - nvm use 0.12.2 | |||
| - ./transform.sh | |||
There was a problem hiding this comment.
Do we want to include running transform.sh to the build process?
There was a problem hiding this comment.
Sorry Maybe I didn't make myself clear in the initial comments. Because this is touching a lot of different places in the code I thought it would be best to have the transformations be done by a script, that way if other stuff is merged in, we don't have to worry as much about merge conflicts.
If I get the needed +1s on the change I will run the script, remove it and any mention of it in the code and check in the result before merging it to master.
I added this line so that travis could test the fully transformed code.
There was a problem hiding this comment.
If you just want to see the fully transformed code I can do that, but it is as of right now it ends up being
593 files changed, 1481 insertions(+), 1481 deletions(-)
There was a problem hiding this comment.
OK then please go ahead. I already run the script and had a look.
|
Left a comment to know intention of changing build process. Others look great. |
|
+1 Nice work. |
|
@HeartSaVioR Thanks I'll make the change |
Because this touches so much of the code to avoid having to constantly upmerge I have a script that will transform most of the code automatically. transform.sh . I don't plan on checking it in. If everyone is OK with the change I will run the script and just check those changes in. The other changes and small updates to make the result of running the script not produce compile errors. Overall this drops more than 800 warnings from the code base (according to eclipse).