You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2017/05/15 20:03:12 UTC
[GitHub] storm pull request #2117: STORM-2515: Fix most checkstyle violations in stor...
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2117
STORM-2515: Fix most checkstyle violations in storm-kafka-client
Most of the remaining violations are a couple of methods missing javadocs in the Trident code, which I don't feel familiar enough with Trident to write. The last couple are because there's a method in KafkaSpoutConfig that contains the SSL abbreviation. The rules only allow 2 capital letters in an abbreviation. Renaming is a breaking change, but maybe this isn't an issue for 2.0?
I also added a commit making a few other changes I thought made sense. The KafkaSpoutMessageId.getMetadata method was including the TopicPartition 3 times. The found variable in OffsetManager.findNextCommitOffset can be replaced with a null check of nextCommitMsg. They are in a separate commit for now, so this is easier to review.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-2515
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2117.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 #2117
----
commit 57fcbbb21728a0b1ccbb41503914b7decc94fceb
Author: Stig Rohde Døssing <st...@gmail.com>
Date: 2017-05-15T19:47:04Z
STORM-2515: Fix most checkstyle violations in storm-kafka-client
----
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo : my opinion runs contrary to others it seems. For every breaking API change my team & company incur a bunch of coordination work. We have many independent internal engineering teams that are entirely responsible for their own topology code, so if there is a breaking API change we need to work with each of them to get them to modify their code appropriately.
Rather than breaking an API to adhere to a belatedly-applied code standard, we could take the approach of allowing explicit exemptions from the uppercase-in-identifier standard via annotations:
* https://stackoverflow.com/a/22556386
This would require another modification to the base checkstyle.xml file, since Google's defaults don't allow this explicit one-off suppression via annotations.
If we don't have the stomach for using this suppression mechanism, then we should at the minimum backport the changed API identifier as an available method in every active storm version branch. That will help minimize upgrade pains since people can make the changes when upgrading to 1.1.x, and not have to make this particular change when upgrading to 2.x.
---
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 #2117: STORM-2515: Fix most checkstyle violations in stor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2117
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@erikdw Sorry, didn't get much response on the mailing list thread, so expected that people just didn't care. I'd be happy with either solution, but prefer deprecating the existing methods and adding the renamed ones in 1.0.x and 1.1.x. Then your teams should be able to fix the deprecation warnings when they have time instead of having to do it immediately.
I don't really feel strongly about it so if you'd prefer going the suppression route let me know, and I'll do that instead. I'd just like to have a general rule for how we resolve this type of violation, since it's an issue in other PRs too (e.g. https://github.com/apache/storm/pull/2146/files)
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo : I might be in the minority (though it's hard to tell, since as you noted the email thread didn't get much traction), but I don't think there's much advantage in changing existing ***external*** APIs just for adherence with this specific coding style standard. I should note that we're not actually using `storm-kafka-client` in our users' topologies yet because we haven't been able to upgrade to Storm 1.0+ yet (partially because of backwards-incompatible changes in code and behavior).
In my view, breaking of backwards compatibility should be done only when actually necessary; e.g., changing package from `backtype` to `org.apache` was a good reason, since we needed to make the project properly namespaced. Whereas coding style adherence is not a very convincing reason.
Maybe someone with a view on the number & scale of breaking changes in Storm 2.0 as compared to 1.0 could speak up? I'm fairly ignorant about the "gestalt" of what is happening in Storm 2.0 and so don't know the answer to that question. i.e., if everyone is going to have to make a bunch of changes *anyways* then it's probably "no big deal" to slap in this method name casing change. But if 2.0 isn't highly divergent as compared to 1.0 for topology authors, then I'd argue this isn't worth it.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo
Since we're already struggling with checkstyle violations, I would like to merge this in, but we should take "breaking change" into consideration.
I'm +1 to 7fdc48a and 49784bf and would like to merge them in via cherry-picking, but would like to see others opinions for f9e3331d with such conventions.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@erikdw That sounds good to me. I'd like to get all the changes done in one PR (plus two for 1.x, 1.0.x) if possible, so we don't have to deal with this again later. Will probably take a look at it sometime this week.
It makes sense to wait to allow suppression until we need it IMO. We can always add it later if we need it.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
If someone wants to take a stab at writing the last bit of javadoc, and renaming the methods in KafkaSpoutConfig is fine, we could get rid of the last few 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.
---
[GitHub] storm issue #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo Sorry not meant as negative for the PR. But want to get a better exposure to everyone on changes that we make and for users/devs who might not be able to follow dev list day-in/day-out and its easy to get buried in the new emails in mailing list. I started discussion around having KIP (Kafka) proposal docs, so that there is a central we are documenting the critical fixes and changes.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo thought I sent and replied your earlier emails as well. Looks like some issue with gmail they are showing as sent but didn't reached the mailing list. I sent them again.
---
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.
---
Re: [GitHub] storm issue #2117: STORM-2515: Fix most checkstyle
violations in storm-kafka...
Posted by Vinod KC <VK...@hortonworks.com>.
I feel , just for check-style issue fix, we shouldn’t change public method names in 1.x
Thanks
Vinod
On 5/21/17, 4:10 PM, "srdo" <gi...@git.apache.org> wrote:
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@erikdw @revans2 @vinodkc You guys might have an opinion on this, since I've seen you discuss checkstyle on other PRs. How do you feel about the method renames in KafkaSpoutConfig in f9e331ddb3e8c1d917a2c9e27c2afbce7ba3dad0?
I think we shouldn't make this kind of breaking change in 1.x, but 2.0 isn't released yet so it's probably OK here?
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@erikdw @revans2 @vinodkc You guys might have an opinion on this, since I've seen you discuss checkstyle on other PRs. How do you feel about the method renames in KafkaSpoutConfig in f9e331ddb3e8c1d917a2c9e27c2afbce7ba3dad0?
I think we shouldn't make this kind of breaking change in 1.x, but 2.0 isn't released yet so it's probably OK here?
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@erikdw Doesn't seem like anyone else is going to chime in. I can't speak for most of the Storm components, but I believe at least storm-kafka-client has a fair number of breaking API changes between 1.x and 2.x. It also seems likely to me that the Java port work would have broken some APIs. Looking at some topology code I have lying around, there's a decent number of compile errors after switching to 2.0.0-SNAPSHOT from 1.1.0
I think if we're going to rename methods it would be good to get them all out of the way in a single release. That way it isn't going to become a regular nuisance. We could make sure to get them all fixed now, and then put deprecations for all of them in 1.x?
I agree that this isn't that important of a change, but my impression is that people are probably going to need to change their code anyway when migrating to 2.x, so I don't know that it adds that much of an additional cost.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo
I think both.
Main issue is "are we OK to introduce breaking change because of method convention?", and I also would like to hear that "is setSslKeystore better than setSSLKeystore".
We may reach consensus easily for latter but maybe not former. Breaking backward compatibility is a bit annoying to the user, and we already did it once for 1.1.0.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo @erikdw
Thanks for the great discussion and like the approach. Please go ahead for the follow up items.
+1 for current PR.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@HeartSaVioR Okay. I started a thread on the mailing list about those issues. Thanks for reviewing.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
The test failure is due to https://github.com/apache/storm/pull/2118
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@harshach Sounds good. Is the discussion on the dev list? I'm not seeing it.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
Reverted the KafkaSpoutConfig method name changes. I'd rather do them in the PR mentioned above.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@HeartSaVioR Sure. Could you start the thread, I'm not sure what the discussion should be? Is it about making breaking changes in 2.0, the method name convention or something else?
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@harshach This PR doesn't (as far as I can tell) contain any breaking changes anymore.
I agree that it would be good to have a better process for polling opinion regarding some changes. This question went on the mailing list and was also discussed here, but the number of participants was very small IMO.
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo Thanks for the quick action!
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2117
@HeartSaVioR How do you feel about merging this? It doesn't seem like anyone was overly worried about the API change in 2.x
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo @HeartSaVioR @erikdw
I understand this PR is merged. But we should be extremely careful when we break the backward incompatibility , if it justifies better implementation of a connector yes but I don't agree on doing this for check style issues. I think we need to propose a better way to get the changes in. We can adopt the KIP style proposals in Storm community that can be discussed and voted upon before we agree. We can skip small changes/bug fixes out of this process and use it bigger changes and back-ward incompatible changes. I started discussion thread we can continue there. 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 feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo
I cherry-picked two commits and lessened max checkstyle violations to 15 because violation count is still 11 after applying two commits.
Could we initiate discussion around `f9e331d` via dev@ mailing list?
---
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 #2117: STORM-2515: Fix most checkstyle violations in storm-kafka...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the issue:
https://github.com/apache/storm/pull/2117
@srdo : thanks for your perspective on this. I wonder if we could do this:
* we adhere to the no-uppercase-acronyms rule we are debating now, thus creating some breaking changes
** as stated previously, we make an effort to backport these changes into storm-1.0.x and/or storm-1.x, keeping both the new name and the old name for the methods, and then we mark the previous methods as deprecated.
* maybe we can add the feature to `storm_checkstyle.xml` that *allows* checkstyle-suppression annotations? Or maybe we just defer that for now.
---
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.
---