Bug Fix 1804 #960

Kunal-Jha · 2018-09-26T15:02:29Z

One-line summary

Zalando ticket : ARUHA-1804

Description

In case of the requesting stats for subscription with stale offsets and show_time_lag, unreported status code is displayed.
Goal: Instead all the stats till the timeout should be returned
Ticket

codecov-io · 2018-09-27T08:51:32Z

Codecov Report

Merging #960 into master will increase coverage by 0.02%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #960      +/-   ##
============================================
+ Coverage     54.06%   54.08%   +0.02%     
  Complexity     1758     1758              
============================================
  Files           320      318       -2     
  Lines          9602     9589      -13     
  Branches        873      872       -1     
============================================
- Hits           5191     5186       -5     
+ Misses         4093     4086       -7     
+ Partials        318      317       -1

Impacted Files	Coverage Δ	Complexity Δ
...kadi/service/subscription/SubscriptionService.java	`72.98% <ø> (ø)`	`38 <0> (ø)`	⬇️
...ando/nakadi/controller/SubscriptionController.java	`76.19% <ø> (+6.62%)`	`7 <0> (ø)`	⬇️
...g/zalando/nakadi/controller/ExceptionHandling.java	`78.12% <ø> (+4.59%)`	`14 <0> (ø)`	⬇️
...rvice/subscription/SubscriptionTimeLagService.java	`79.1% <66.66%> (+1.32%)`	`11 <1> (ø)`	⬇️
...ns/runtime/ErrorGettingCursorTimeLagException.java	`0% <0%> (-80%)`	`0% <0%> (-1%)`
...n/java/org/zalando/nakadi/service/EventStream.java	`74.79% <0%> (+1.62%)`	`30% <0%> (+1%)`	⬆️

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 885a8c8...756fd9e. Read the comment docs.

rcillo · 2018-10-04T11:41:43Z

@Kunal-Jha please, add some more information to the PR description and update it following the template. It should have the Jira ticket link as a minimum requirement, when there is a correspondent jira ticket.

If you don't have all the information required by the template, please delete such info from the template.

rcillo · 2018-10-04T13:27:23Z

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java

+                    try {
+                        timeLagFuture   = timeLagHandler.getCursorTimeLagFuture(cursor);
+                    } catch (TimeoutException e) {
+                        LOG.error("Time lag request timed out");


I'm afraid this doesn't fix all possible problems regarding timeouts.

This exception you silenced here would be thrown in case the semaphore tryAcquire fails. But we could also have another timeout in case the future was successfully scheduled for execution but did not finish within the expected time.

This is handled by lines 88/95. So I think that a fix for this should make sure that it doesn't launch 422 in case of failed time lag computation, be it in the semaphore or the future by itself.

…into aruha-1804-bugfix

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java

rcillo · 2018-10-08T12:05:02Z

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java

+        } catch (RejectedExecutionException|TimeoutException | ExecutionException  e) {
+            LOG.warn("caught exception the timelag stats are not complete");
+        } catch (Throwable e){
+            LOG.warn("caught throwable the timelag stats are not complete");


Please, log the exception as well.

rcillo · 2018-10-08T12:21:54Z

@Kunal-Jha now that we are no longer throwing those exception, I did a search in the project to see if they were used somewhere else and found out that they are handled by ExceptionHandling class. Since they are no longer thrown, I would request you to remove the exception handling from this class as well as delete the exception class itself.

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java

src/test/java/org/zalando/nakadi/service/SubscriptionTimeLagServiceTest.java

rcillo · 2018-10-12T14:37:11Z

@Kunal-Jha please, rebase/merge this branch and I'll approve.

rcillo · 2018-10-12T16:05:00Z

👍

Kunal-Jha · 2018-10-15T08:24:26Z

deploy validation please

rcillo · 2018-10-15T12:26:15Z

@Kunal-Jha during validation I realised we forgot to update the documentation for this attribute. Please change the description for show_time_lag to better explain the behaviour. Right now it says show consumer time lag but this is not longer true. It might or might not show it depending on whether a problems happens (timeout or anything else). So we better write something like shows consumer time lag as an optional field. This option is time consuming and Nakadi attempts to compute it in a best effort strategy. In case Nakadi is not able to compute it within a configurable timeout, the field might not be present in the output.. But the final words are yours, this is just a suggestion.

rcillo · 2018-10-15T14:15:24Z

docs/_data/nakadi-event-bus-api.yaml

@@ -1442,7 +1442,10 @@ paths:
        - $ref: '#/parameters/SubscriptionId'
        - name: show_time_lag
          in: query
-          description: show consumer time lag
+          description: shows consumer time lag as an optional field.


I think you need to use the "|" for paragraphs in OpenAPI, please check an example where you can find description: |

rcillo · 2018-10-15T14:22:46Z

docs/_data/nakadi-event-bus-api.yaml

@@ -1445,7 +1445,8 @@ paths:
          description: shows consumer time lag as an optional field.
          This option is a time consuming operation and Nakadi attempts to compute it in the best possible strategy.
          In cases of failures, resulting in Nakadi being unable to compute it within a configurable timeout,
-          the field might not be present in the output.
+          the field might either be partially present or not present (dependiong on the number of successful requests)


s/dependiong/depending/

rcillo · 2018-10-15T14:49:04Z

docs/_data/nakadi-event-bus-api.yaml

@@ -1442,10 +1442,11 @@ paths:
        - $ref: '#/parameters/SubscriptionId'
        - name: show_time_lag
          in: query
-          description: shows consumer time lag as an optional field.
+          description: |
+          Shows consumer time lag as an optional field.


You need to ident the content with 2 extra spaces.

rcillo · 2018-10-15T14:59:25Z

👍

Kunal-Jha · 2018-10-15T15:17:01Z

👍

Bug Fix 1804

8480c06

Kunal-Jha requested a review from a team as a code owner September 26, 2018 15:02

Changed the error fix to each timelag instead of whole batch

4571dcc

Kunal Jha and others added 2 commits September 27, 2018 10:54

Removed the unncessary changes

c5c4bbe

Update SubscriptionService.java

863f777

rcillo reviewed Oct 4, 2018

View reviewed changes

Kunal Jha added 2 commits October 4, 2018 19:05

Added changes from the review

5fc8c23

Merge branch 'aruha-1804-bugfix' of https://github.com/zalando/nakadi …

8832d46

…into aruha-1804-bugfix

rcillo reviewed Oct 5, 2018

View reviewed changes

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java Show resolved Hide resolved

test changes

e8dfa08

rcillo reviewed Oct 5, 2018

View reviewed changes

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java Outdated Show resolved Hide resolved

rcillo reviewed Oct 8, 2018

View reviewed changes

added Review points

06c9961

removed exception which are not used

42df018

v-stepanov reviewed Oct 10, 2018

View reviewed changes

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java Outdated Show resolved Hide resolved

v-stepanov reviewed Oct 10, 2018

View reviewed changes

src/main/java/org/zalando/nakadi/service/subscription/SubscriptionTimeLagService.java Outdated Show resolved Hide resolved

v-stepanov reviewed Oct 10, 2018

View reviewed changes

src/test/java/org/zalando/nakadi/service/SubscriptionTimeLagServiceTest.java Outdated Show resolved Hide resolved

v-stepanov reviewed Oct 10, 2018

View reviewed changes

src/test/java/org/zalando/nakadi/service/SubscriptionTimeLagServiceTest.java Outdated Show resolved Hide resolved

Fixed reviews

6fb3731

merge conflicts fixed

41406c6

added description

c4fe267

rcillo reviewed Oct 15, 2018

View reviewed changes

updated description

4c1758b

rcillo reviewed Oct 15, 2018

View reviewed changes

fixed errors

7a85837

rcillo reviewed Oct 15, 2018

View reviewed changes

spacing for description fixed

756fd9e

rcillo approved these changes Oct 15, 2018

View reviewed changes

Kunal-Jha merged commit 3f65fd8 into master Oct 15, 2018

Kunal-Jha deleted the aruha-1804-bugfix branch October 15, 2018 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix 1804 #960

Bug Fix 1804 #960

Kunal-Jha commented Sep 26, 2018 •

edited

Loading

codecov-io commented Sep 27, 2018 •

edited

Loading

rcillo commented Oct 4, 2018

rcillo Oct 4, 2018

rcillo Oct 8, 2018

rcillo commented Oct 8, 2018 •

edited

Loading

rcillo commented Oct 12, 2018

rcillo commented Oct 12, 2018

Kunal-Jha commented Oct 15, 2018

rcillo commented Oct 15, 2018

rcillo Oct 15, 2018

rcillo Oct 15, 2018

rcillo Oct 15, 2018

rcillo commented Oct 15, 2018

Kunal-Jha commented Oct 15, 2018

Bug Fix 1804 #960

Bug Fix 1804 #960

Conversation

Kunal-Jha commented Sep 26, 2018 • edited Loading

One-line summary

Description

codecov-io commented Sep 27, 2018 • edited Loading

Codecov Report

rcillo commented Oct 4, 2018

rcillo Oct 4, 2018

Choose a reason for hiding this comment

rcillo Oct 8, 2018

Choose a reason for hiding this comment

rcillo commented Oct 8, 2018 • edited Loading

rcillo commented Oct 12, 2018

rcillo commented Oct 12, 2018

Kunal-Jha commented Oct 15, 2018

rcillo commented Oct 15, 2018

rcillo Oct 15, 2018

Choose a reason for hiding this comment

rcillo Oct 15, 2018

Choose a reason for hiding this comment

rcillo Oct 15, 2018

Choose a reason for hiding this comment

rcillo commented Oct 15, 2018

Kunal-Jha commented Oct 15, 2018

Kunal-Jha commented Sep 26, 2018 •

edited

Loading

codecov-io commented Sep 27, 2018 •

edited

Loading

rcillo commented Oct 8, 2018 •

edited

Loading