Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STORM-2295:KafkaSpoutStreamsNamedTopics changing the sequence of fields name while emitting data #1878

Closed
wants to merge 5 commits into from
Closed

STORM-2295:KafkaSpoutStreamsNamedTopics changing the sequence of fields name while emitting data #1878

wants to merge 5 commits into from

Conversation

pasalkarsachin1
Copy link
Contributor

@pasalkarsachin1 pasalkarsachin1 commented Jan 16, 2017 •
edited
Loading

STORM-2295: I have just updated code to make sure sequence Fields should remains same even its added in set. In old implementation because of HashSet it was changing. Also added testcase for the same

@pasalkarsachin1 pasalkarsachin1 changed the title Issue Fixed STORM-2295:Issue Fixed Jan 16, 2017
@HeartSaVioR
Copy link
Contributor

@pasalkarsachin1
Please change the PR's title to same as issue's title or at least summary of this PR.

@pasalkarsachin1 pasalkarsachin1 changed the title STORM-2295:Issue Fixed STORM-2295:KafkaSpoutStreamsNamedTopics changing the sequence of fields name while emitting data Jan 17, 2017
@pasalkarsachin1
Copy link
Contributor Author

@HeartSaVioR
Done. Thanks.

@revans2
Copy link
Contributor

revans2 commented Jan 17, 2017

+1 the change looks good to me. Great catch @pasalkarsachin1

@hmcl
Copy link
Contributor

hmcl commented Jan 17, 2017

+1

@pasalkarsachin1
Copy link
Contributor Author

Can someone merge it?

harshach
harshach reviewed Jan 19, 2017
View reviewed changes
...afka-client/src/test/java/org/apache/storm/kafka/spout/KafkaSpoutStreamsNamedTopicsTest.java
@@ -0,0 +1,23 @@
package org.apache.storm.kafka.spout;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the Apache License

@harshach
Copy link
Contributor

@pasalkarsachin1 one of the files missing the License info. Please add that. We can merge it in after that.

@HeartSaVioR
Copy link
Contributor

@pasalkarsachin1
And also please squash the commits into one after fixing license info.
I recommend the title of commit message as STORM-2295 KafkaSpoutStreamsNamedTopics changing the sequence of fields name while emitting data and abstracted description to the body.

@hmcl
Copy link
Contributor

hmcl commented Jan 19, 2017

I would suggest for title STORM-2295 KafkaSpoutStreamsNamedTopics should return output fields with predictable ordering

@pasalkarsachin1
STORM-2295 KafkaSpoutStreamsNamedTopics should return output fields w…
889dbff 
…ith predictable ordering
@pasalkarsachin1
STORM-2295 KafkaSpoutStreamsNamedTopics should return output fields w…
7168710 
…ith predictable ordering

KafkaSpoutStreamsNamedTopics.getOutputFields() uses HashSet causes output fields with predictable ordering. So replaced with LinkedHashSet
@pasalkarsachin1
Merge branch 'STORM-2295' of https://github.com/pasalkarsachin1/storm …
5ed8088 
…into STORM-2295
@pasalkarsachin1
Copy link
Contributor Author

I squashed it.

@HeartSaVioR
Copy link
Contributor

@pasalkarsachin1
PR has been applied to master via ba406c1 and 1.x-branch via f4fdab3.
(I just cherry-picked 7168710 since it's squashed commit and others are not necessary.)

Could you please close this issue? Thanks in advance!
(Github doesn't recognize auto PR close message which PR is not target to master branch.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshach harshach harshach left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pasalkarsachin1 @HeartSaVioR @revans2 @hmcl @harshach