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

Implement light subscriptions stats and refactor #861

Merged
merged 15 commits into from Apr 25, 2018

Conversation

5 participants
@lmontrieux
Member

lmontrieux commented Apr 15, 2018

Implement light subscriptions stats and refactor

Zalando ticket : ARUHA-1551

Description

  • light subscription stats in /subscriptions endpoint
  • refactor Zk subscription client to not acquire locks to read
    subscription data
  • remove ServiceUnavailableException from the project, preferring the
    unchecked ServiceTemporarilyUnavailableException instead
  • update the swagger definition

Review

  • Tests
  • Documentation
  • CHANGELOG

lmontrieux added some commits Apr 5, 2018

Implement light subscriptions stats and refactor
- light subscription stats in /subscriptions endpoint
- refactor Zk subscription client to not acquire locks to read
subscription data
- remove ServiceUnavailableException from the project, preferring the
unchecked ServiceTemporarilyUnavailableException instead
- update the swagger definition
@codecov-io

This comment has been minimized.

codecov-io commented Apr 15, 2018

Codecov Report

Merging #861 into master will decrease coverage by 0.03%.
The diff coverage is 62.9%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #861      +/-   ##
============================================
- Coverage     53.59%   53.56%   -0.04%     
- Complexity     1680     1691      +11     
============================================
  Files           310      309       -1     
  Lines          9327     9359      +32     
  Branches        837      841       +4     
============================================
+ Hits           4999     5013      +14     
- Misses         4029     4046      +17     
- Partials        299      300       +1
Impacted Files Coverage Δ Complexity Δ
...alando/nakadi/service/CursorOperationsService.java 59.8% <ø> (ø) 21 <0> (ø) ⬇️
...org/zalando/nakadi/repository/TopicRepository.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...nakadi/service/converter/VersionZeroConverter.java 29.31% <ø> (ø) 5 <0> (ø) ⬇️
.../nakadi/service/converter/CursorConverterImpl.java 51.42% <ø> (ø) 7 <0> (ø) ⬇️
...ava/org/zalando/nakadi/service/CursorsService.java 10.3% <ø> (ø) 2 <0> (ø) ⬇️
.../service/subscription/zk/ZkSubscriptionClient.java 31.7% <ø> (ø) 0 <0> (ø) ⬇️
...va/org/zalando/nakadi/service/CursorConverter.java 87.5% <ø> (ø) 0 <0> (ø) ⬇️
...alando/nakadi/controller/PartitionsController.java 79.22% <ø> (-7.8%) 12 <0> (ø)
...g/zalando/nakadi/controller/ExceptionHandling.java 52.5% <ø> (ø) 12 <0> (ø) ⬇️
...a/org/zalando/nakadi/service/BlacklistService.java 13.04% <0%> (ø) 3 <0> (ø) ⬇️
... and 21 more

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 25e9497...e06f28d. Read the comment docs.

@@ -964,6 +964,11 @@ paths:
required: false
default: 0
minimum: 0
- name: show_status

This comment has been minimized.

@adyach

adyach Apr 16, 2018

Member

do you think it is better to have separate endpoint for that ?

This comment has been minimized.

@lmontrieux

lmontrieux Apr 16, 2018

Member

@adyach I followed the requirements in our internal ticket:
GET /subscriptions endpoint should have additional parameter show_status = true/false (false by default).

To be honest I don't really have a preference one way or another. Do you think it would be better as a separate endpoint?

type: object
description: statistics of partition within a subscription context
properties:
partition:

This comment has been minimized.

@v-stepanov

v-stepanov Apr 16, 2018

Member

there's no description for partition field

This comment has been minimized.

@lmontrieux
throws InconsistentStateException, ServiceTemporarilyUnavailableException {
final List<EventType> eventTypes = getEventTypesForSubscription(subscription);
final List<PartitionEndStatistics> topicPartitions = loadPartitionEndStatistics(eventTypes);

This comment has been minimized.

@v-stepanov

v-stepanov Apr 16, 2018

Member

As I see we do this call not looking at includeDistance value. But this method will actually go to Kafka and do this expensive seek operation. I thought that in case of light-stats we will not check the offsets in Kafka.

This comment has been minimized.

@lmontrieux

lmontrieux Apr 16, 2018

Member

That is correct, I thought of doing it then forgot to do so. Looks like there will be some more refactoring.

This comment has been minimized.

@lmontrieux
throws SubscriptionNotInitializedException, NakadiRuntimeException {
if (!isSubscriptionCreatedAndInitialized()) {
return Optional.empty();
}
return Optional.of(runLocked(() -> new ZkSubscriptionNode(

This comment has been minimized.

@v-stepanov

v-stepanov Apr 16, 2018

Member

Why have you removed running under lock?

This comment has been minimized.

@lmontrieux

lmontrieux Apr 16, 2018

Member

Because it isn't necessary anymore, since we migrated to new zk subscription format (I double checked with @antban )

lmontrieux added some commits Apr 16, 2018

if (includeDistance) {
return loadStats(eventTypes, zkSubscriptionNode, subscriptionClient);
}
else {

This comment has been minimized.

@v-stepanov

v-stepanov Apr 17, 2018

Member

I used to put else on a new line like that but the team told me not to do that :(

This comment has been minimized.

@lmontrieux

lmontrieux Apr 17, 2018

Member

@v-stepanov faaee92. What the team wants, the team gets :)

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 18, 2018

@lmontrieux code looks good for me now.
Are there any tests for the new functionality?

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 18, 2018

deploy validation please

@@ -18,6 +20,8 @@ public Subscription(final String id, final DateTime createdAt, final Subscriptio
private DateTime createdAt;
private List<SubscriptionEventTypeStats> stats;

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

maybe you can add @JsonInclude(Include.NON_NULL) here? Because in other case it will show up for all endpoints where we return subscription.

This comment has been minimized.

@lmontrieux
@@ -34,6 +38,14 @@ public void setCreatedAt(final DateTime createdAt) {
this.createdAt = createdAt;
}
public List<SubscriptionEventTypeStats> getStats() {

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

probably it should be marked as nullable as it is null in most of the cases

This comment has been minimized.

@lmontrieux
@@ -157,10 +157,16 @@ public Result listSubscriptions(@Nullable final String owningApplication, @Nulla
subscriptionRepository.listSubscriptions(eventTypesFilter, owningAppOption, offset, limit);
final PaginationLinks paginationLinks = SubscriptionsUriHelper.createSubscriptionPaginationLinks(
owningAppOption, eventTypesFilter, offset, limit, subscriptions.size());

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

new field show_status should also be included to pagination link.

This comment has been minimized.

@lmontrieux

lmontrieux added some commits Apr 19, 2018

@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 19, 2018

@v-stepanov there are 2 acceptance tests in 2ffeb20

@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 19, 2018

👍

lmontrieux added some commits Apr 19, 2018

public void whenLightStatsOnNotInitializedSubscriptionThenCorrectResponse() throws IOException {
final String et = createEventType().getName();
final Subscription s = createSubscriptionForEventType(et);
final Response response = when().get("/subscriptions?show_status=true").thenReturn();

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

it's an acceptance test, what if there will be other subscriptions created by other tests? I think it's better to differ the subscription you created (e.g. by owning_application)

This comment has been minimized.

@lmontrieux
if (subscription.getId().equals(s.getId())) {
Assert.assertNotNull(subscription.getStats());
Assert.assertEquals("assigned", subscription.getStats().get(0).getPartitions().get(0).getState());
Assert.assertNotEquals("", subscription.getStats().get(0).getPartitions().get(0).getStreamId());

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

you can actually take streamId from TestStreamingClient and use it here for assertion.

This comment has been minimized.

@lmontrieux
.start();
waitFor(() -> assertThat(client.getBatches(), hasSize(15)));
final Response response = when().get("/subscriptions?show_status=true").thenReturn();

This comment has been minimized.

@v-stepanov

v-stepanov Apr 19, 2018

Member

here it's also possible to have the same problem as I described several lines ago in another test (subscriptions left by other tests).

This comment has been minimized.

@lmontrieux

@lmontrieux lmontrieux changed the title from [WIP] Implement light subscriptions stats and refactor to Implement light subscriptions stats and refactor Apr 19, 2018

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 19, 2018

👍

1 similar comment
@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 19, 2018

👍

@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 19, 2018

Deploy validation please

lmontrieux added some commits Apr 20, 2018

Change wording in documentation from light stats to status
This is to avoid confusion, since the flag is called show_status,
and the result is the status of the subscription, not some stats
@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 20, 2018

👍

@lmontrieux

This comment has been minimized.

Member

lmontrieux commented Apr 20, 2018

👍

1 similar comment
@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 20, 2018

👍

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 23, 2018

deploy validation please

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Apr 25, 2018

👍

1 similar comment
@antban

This comment has been minimized.

Member

antban commented Apr 25, 2018

👍

@v-stepanov v-stepanov merged commit f56de8a into master Apr 25, 2018

6 of 7 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
codecov/patch 62.9% of diff hit (target 53.59%)
Details
codecov/project Absolute coverage decreased by -0.03% but relative coverage increased by +9.3% compared to 25e9497
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr Approvals: @v-stepanov, @antban.
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