Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR : FYI, here's the PR for the backport:
* https://github.com/apache/storm/pull/2188
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR : FYI, I resolved the conflicts in my local repo, I'm gonna
build and then send a new PR, will link from here.
---
If your project is set up for it, you can reply to this email and have
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR : I didn't have time tonight. I'll try to get to it tomorrow.
---
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
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw Nice! Thank you.
---
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
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR : I'll try to take a stab later tonight at it. I'll let you
know if I find time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2109
I'll try to resolve the conflict on top of this patch when I have free
time. If someone volunteers and raise a new PR it would be great.
---
If your project is set up for it, you can reply to
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2109
Not sure, but we may need to have bug fix version soon since there're some
opened pull requests on storm-kafka-client which sound like critical.
---
If your project is set up for it, you can
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR : thanks a lot! @srishtyagrawal is on leave for a month. Are
there any 1.x releases planned before late July?
---
If your project is set up for it, you can reply to this email and
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2109
@srishtyagrawal
I merged this to master branch, but seems like there're some conflicts
against 1.x branch. If you want to adopt this to 1.x version line, you may need
to raise a new PR
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@HeartSaVioR thanks for the approval. I have rebased the changes on top of
the latest master branch. Can you please merge this PR?
---
If your project is set up for it, you can reply to this
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2109
+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
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@harshach can you please help with merging this PR? It has been sitting for
2 weeks now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
Thanks :)
---
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
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
[Filed a new ticket to track this change for
trident.](https://issues.apache.org/jira/browse/STORM-2530)
---
If your project is set up for it, you can reply to this email and have your
reply
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
@srishtyagrawal I think you are right, it probably makes sense to add. It
looks like the Trident spout supports the same subscription methods as the
regular spout, so it would probably be helpful to
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@srdo I was thinking of it from the perspective of consistency (between the
trident KafkaSpout and normal KafkaSpout), not sure how useful it will be
(don't know much about trident).
---
If
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
@srishtyagrawal If you think it would be helpful? I don't really have an
opinion about that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@revans2 can you please merge this PR?
@srdo do we need task-ID values in logs while printing partition to task
mapping for trident? I can file another STORM ticket for that.
---
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
+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
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@srdo let me know if further changes are required.
---
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
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
[Green
âï¸](https://travis-ci.org/apache/storm/builds/233734011?utm_source=github_status_medium=notification),
woot!
---
If your project is set up for it, you can reply to this email and have
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw thanks for pointing that out. Rebasing on top of the recent changes
in master helped with the integration tests.
---
If your project is set up for it, you can reply to this email and
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@srishtyagrawal : thanks! So there's still a [test
failure](https://travis-ci.org/apache/storm/jobs/233447284):
```
Tests run: 37, Failures: 1, Errors: 0, Skipped: 0, Time elapsed:
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw @srdo fixed the code according to your suggestions.
---
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
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
Thanks @erikdw for clarifying that. I was assuming that it must be a bad
coding practice to have same name for a method as well as variable. Even
`taskPrefix` is a method as well as variable
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
technically and I think canonically, there's no problem with a method and a
variable having the same exact name. What's bad in the existing code is that
`taskId()` means something *entirely*
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw's suggestion on renaming the function `taskId()` to `taskPrefix()`
seems reasonable to me. I can then rename the variable name from `taskID` to
`taskId`.
---
If your project is
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@srdo : ah, I forgot that `final` was different from `static` + `final`,
and that `static` + `final` is *really* what a constant should be.
So, in that list that I [posted
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw Isn't naming variables e.g. `FOO_BAR` usually reserved for
constants that are both static and final? I wouldn't usually expect non-static
finals to be all caps. The `ignoreStatic` default of
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@revans2 : agree that we should be careful in making changes away from the
base "google_checks.xml", and I did already [argue
above](https://github.com/apache/storm/pull/2109#discussion_r116677826)
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw Thanks for pointing out the defaults in google_checks.xml. I am not
sure what the reason is behind deviating from the checkstyle defaults.
@revans2 variable `taskID` has been
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw and @srishtyagrawal I know that we are still working through issues
with checkstyle being new, but I get a little nervous with making too many
changes to the "standard".
The more
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
@srishtyagrawal : we didn't *choose* to set it to 1, we just inherited the
default of the google style in
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw @vinodkc @revans2 @srdo
I am encountering the following checkstyle error while building my code :
```
```
This error, I figured is because of the
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2109
These failed build errors should go away once #2112 (my checkstyle-fixing
PR) is merged and you rebase.
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user srishtyagrawal commented on the issue:
https://github.com/apache/storm/pull/2109
@erikdw @revans2 @harshach Please review and let me know if any changes are
required.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2109
LGTM. The dummy values in the tests seem fine. The proposed extra changes
sound reasonable.
The commit message is referring to another (internal?) issue. Could you
update it to refer to
37 matches
Mail list logo
