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.

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

Remove legacy feature toggles #914

Merged
merged 9 commits into from Jul 31, 2018
Merged

Remove legacy feature toggles #914

merged 9 commits into from Jul 31, 2018

Conversation

@ferbncode
Copy link
Member

ferbncode commented Jul 20, 2018

Remove legacy feature toggles to control the tech debit in order to not slowdown the development process.

Zalando ticket : ARUHA-1736

Description

The following feature toggles have been removed:

  • CHECK_PARTITIONS_KEYS
  • CHECK_OWNING_APPLICATION
  • LIMIT_CONSUMERS_NUMBER
  • SEND_BATCH_VIA_OUTPUT_STREAM
Remove legacy feature toggles to control the tech debit in order to not slowdown the development process.
@lmontrieux
Copy link
Member

lmontrieux commented Jul 20, 2018

@ferbncode one unit test is failing, could you please have a look?

@@ -244,13 +243,11 @@ public StreamingResponseBody streamEvents(
.build();

// acquire connection slots to limit the number of simultaneous connections from one client
if (featureToggleService.isFeatureEnabled(LIMIT_CONSUMERS_NUMBER)) {

This comment has been minimized.

@v-stepanov

v-stepanov Jul 20, 2018 Contributor

@ferbncode
Please check the actual status (ON/OFF) on live/staging systems of each feature-toggle you remove.
For example, this one - LIMIT_CONSUMERS_NUMBER is currently OFF on all environments, but after your changes it will become always ON.

This comment has been minimized.

@rcillo

rcillo Jul 20, 2018 Member

@ferbncode in order to check if a feature toggle is turned ON/OFF you can use the following endpoints:

TOKEN=`zign token`
export NAKADI_URL=https://nakadi-staging.aruha-test.zalan.do
curl -v -XGET -H "Authorization: Bearer ${TOKEN}" $NAKADI_URL/settings/features | jq
@ferbncode ferbncode force-pushed the ferbncode:ARUHA-1736 branch from 2876668 to a1021fa Jul 22, 2018
@codecov-io
Copy link

codecov-io commented Jul 22, 2018

Codecov Report

Merging #914 into master will increase coverage by 0.67%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #914      +/-   ##
===========================================
+ Coverage     53.63%   54.3%   +0.67%     
+ Complexity     1777    1770       -7     
===========================================
  Files           323     319       -4     
  Lines          9854    9678     -176     
  Branches        902     886      -16     
===========================================
- Hits           5285    5256      -29     
+ Misses         4234    4091     -143     
+ Partials        335     331       -4
Impacted Files Coverage Δ Complexity Δ
...alando/nakadi/service/EventStreamWriterBinary.java 94.66% <ø> (ø) 8 <0> (ø) ⬇️
...zalando/nakadi/controller/EventTypeController.java 90.74% <ø> (+4.77%) 22 <0> (ø) ⬇️
...g/zalando/nakadi/service/FeatureToggleService.java 46.42% <ø> (-6.7%) 0 <0> (ø)
...lando/nakadi/controller/EventStreamController.java 85.71% <ø> (+3.04%) 21 <0> (ø) ⬇️
.../nakadi/controller/PostSubscriptionController.java 92% <ø> (+2.71%) 8 <0> (-1) ⬇️
...org/zalando/nakadi/service/EventStreamFactory.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...n/java/org/zalando/nakadi/service/EventStream.java 75% <100%> (+1.61%) 30 <1> (+1) ⬆️
...a/org/zalando/nakadi/service/EventTypeService.java 76.1% <100%> (-0.09%) 60 <0> (-1)
...vice/subscription/SubscriptionStreamerFactory.java 37.77% <50%> (ø) 1 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c4631...dfcfa48. Read the comment docs.

@adyach
Copy link
Member

adyach commented Jul 23, 2018

LGTM, I suggest to open another PR to remove the code, which we do not use anymore, e.g.: EventStreamWriterString, ConsumerLimitingService.

@@ -23,13 +24,13 @@
@Autowired
public EventStreamFactory(
final CursorConverter cursorConverter,
final EventStreamWriterProvider writerProvider,
@Qualifier("binary") final EventStreamWriter eventStreamWriter,

This comment has been minimized.

@rcillo

rcillo Jul 23, 2018 Member

@ferbncode I guess that this @Qualifier is no longer needed since there is only one implementation of EventStreamWriter now. Could you please check it? If so, then remove it.

@rcillo
Copy link
Member

rcillo commented Jul 23, 2018

👍

@rcillo
Copy link
Member

rcillo commented Jul 23, 2018

@ferbncode congrats on your first contribution to Nakadi 😄 👏

@lmontrieux
Copy link
Member

lmontrieux commented Jul 23, 2018

👎

@@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [2.8.1]

### Removed

This comment has been minimized.

@lmontrieux

lmontrieux Jul 23, 2018 Member

this should be under [Unreleased], because 2.8.1 has been released already

@lmontrieux
Copy link
Member

lmontrieux commented Jul 23, 2018

@ferbncode good work, and congrats on your first contribution. But there is one last small thing to change. I wrote a comment a moment ago.

@rcillo
Copy link
Member

rcillo commented Jul 23, 2018

👍

1 similar comment
@lmontrieux
Copy link
Member

lmontrieux commented Jul 23, 2018

👍

@ferbncode
Copy link
Member Author

ferbncode commented Jul 23, 2018

deploy validation please

@lmontrieux
Copy link
Member

lmontrieux commented Jul 26, 2018

👍

1 similar comment
@ferbncode
Copy link
Member Author

ferbncode commented Jul 26, 2018

👍

@ferbncode ferbncode closed this Jul 27, 2018
@ferbncode ferbncode reopened this Jul 27, 2018
@ferbncode ferbncode closed this Jul 30, 2018
@ferbncode ferbncode reopened this Jul 30, 2018
@ferbncode ferbncode merged commit 9a92d5b into zalando:master Jul 31, 2018
6 checks passed
6 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch 66.66% of diff hit (target 53.63%)
Details
codecov/project 54.3% (+0.67%) compared to 24c4631
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
zappr Approvals: @lmontrieux, @ferbncode.
zappr/pr/specification PR has passed specification checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.