Kafka
  1. Kafka
  2. KAFKA-738

correlationId is not set in FetchRequest in AbstractFetcherThread

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.8.0
    • Fix Version/s: 0.8.0
    • Component/s: core
    • Labels:

      Description

      correlationId is always 0 in FetchRequest in AbstractFetcherThread.

      1. kafka-738_v3.patch
        2 kB
        Jun Rao
      2. kafka-738.patch
        6 kB
        Jun Rao
      3. kafka-738_v2.patch
        5 kB
        Jun Rao
      4. kafka-738.patch
        3 kB
        Jun Rao

        Activity

        Jun Rao created issue -
        Hide
        Swapnil Ghike added a comment -

        Probably because we create a new FetchRequestBuilder object in AbstractFetcherThread.doWork() and create the fetchRequest as

        val fetchRequest = fetchRequestBuilder.build() // this sets correlationId in fetchRequest to 0.

        Show
        Swapnil Ghike added a comment - Probably because we create a new FetchRequestBuilder object in AbstractFetcherThread.doWork() and create the fetchRequest as val fetchRequest = fetchRequestBuilder.build() // this sets correlationId in fetchRequest to 0.
        Hide
        Jun Rao added a comment -

        Attach a patch.

        Yes, we have to create a new FetchRequestBuilder every time, since some of the old values need to be cleared.

        Show
        Jun Rao added a comment - Attach a patch. Yes, we have to create a new FetchRequestBuilder every time, since some of the old values need to be cleared.
        Jun Rao made changes -
        Field Original Value New Value
        Attachment kafka-738.patch [ 12566842 ]
        Jun Rao made changes -
        Assignee Jun Rao [ junrao ]
        Jun Rao made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Neha Narkhede made changes -
        Labels p2
        Hide
        Swapnil Ghike added a comment -

        With this patch, all fetchRequests in SimpleConsumerShell will have correlationId = 0. This will happen in a bunch of unit tests too.
        A solution could be to use the same fetchRequestBuilder in AbstractFetcherThread, but clear its old values every time in doWork(). This way we can make sure that a fetchRequestBuilder can increment its own correlationId with every call to build().

        Show
        Swapnil Ghike added a comment - With this patch, all fetchRequests in SimpleConsumerShell will have correlationId = 0. This will happen in a bunch of unit tests too. A solution could be to use the same fetchRequestBuilder in AbstractFetcherThread, but clear its old values every time in doWork(). This way we can make sure that a fetchRequestBuilder can increment its own correlationId with every call to build().
        Hide
        Jun Rao added a comment -

        Attach patch v2 that fixes the simpleConsumerShell issue. Overall, it seems it's safer if we don't reuse the request builder.

        Show
        Jun Rao added a comment - Attach patch v2 that fixes the simpleConsumerShell issue. Overall, it seems it's safer if we don't reuse the request builder.
        Jun Rao made changes -
        Attachment kafka-738_v2.patch [ 12566857 ]
        Hide
        Swapnil Ghike added a comment -

        I see. Actually the same issue is present in hadoop-consumer's use of builder.build(). Unit tests look fine with patch v2.

        Show
        Swapnil Ghike added a comment - I see. Actually the same issue is present in hadoop-consumer's use of builder.build(). Unit tests look fine with patch v2.
        Hide
        Neha Narkhede added a comment -

        We don't use the builder pattern the way it is supposed to be used. The builder is created once, setting the required values and calling build() happens multiple times. In our case, this is complicated because we maintain some state in the request map, but this can easily be resolved by providing a resetBuilder() API. That way, it will work for SimpleConsumer and high level consumer

        Show
        Neha Narkhede added a comment - We don't use the builder pattern the way it is supposed to be used. The builder is created once, setting the required values and calling build() happens multiple times. In our case, this is complicated because we maintain some state in the request map, but this can easily be resolved by providing a resetBuilder() API. That way, it will work for SimpleConsumer and high level consumer
        Hide
        Jun Rao added a comment -

        Attach patch v3. Having a separate reset() method adds more work to the client. Changed it to reset the map after build().

        Show
        Jun Rao added a comment - Attach patch v3. Having a separate reset() method adds more work to the client. Changed it to reset the map after build().
        Jun Rao made changes -
        Attachment kafka-738.patch [ 12567018 ]
        Hide
        Neha Narkhede added a comment -

        Thanks for the new patch. Some more comments -

        1. FetchRequest
        1.1 Remove unused import import java.util.concurrent.atomic.AtomicInteger
        1.2 I thought you wanted to clear the map in the build method. Don't see that change included in this patch ?

        2. FetchRequestBuilder
        2.1 It is not ideal to have the SimpleConsumer set the correlation id correctly on each fetch request. It is better to leave it as it was done before (increment the correlation id in build()) and just change it to use Utils.getNextNonNegativeInt.

        Show
        Neha Narkhede added a comment - Thanks for the new patch. Some more comments - 1. FetchRequest 1.1 Remove unused import import java.util.concurrent.atomic.AtomicInteger 1.2 I thought you wanted to clear the map in the build method. Don't see that change included in this patch ? 2. FetchRequestBuilder 2.1 It is not ideal to have the SimpleConsumer set the correlation id correctly on each fetch request. It is better to leave it as it was done before (increment the correlation id in build()) and just change it to use Utils.getNextNonNegativeInt.
        Hide
        Jun Rao added a comment -

        Sorry, attached the wrong patch. Attach the correct one v3 this time.

        Show
        Jun Rao added a comment - Sorry, attached the wrong patch. Attach the correct one v3 this time.
        Jun Rao made changes -
        Attachment kafka-738_v3.patch [ 12567044 ]
        Hide
        Neha Narkhede added a comment -

        +1 on the latest patch

        Show
        Neha Narkhede added a comment - +1 on the latest patch
        Hide
        Swapnil Ghike added a comment -

        Though with this patch, the requestMap will be cleared in SimpleConsumerShell and other places that use build(), which is not the case currently.

        Show
        Swapnil Ghike added a comment - Though with this patch, the requestMap will be cleared in SimpleConsumerShell and other places that use build(), which is not the case currently.
        Hide
        Swapnil Ghike added a comment -

        +1

        Show
        Swapnil Ghike added a comment - +1
        Hide
        Jun Rao added a comment -

        Thanks for the review. Committed to 0.8.

        Show
        Jun Rao added a comment - Thanks for the review. Committed to 0.8.
        Jun Rao made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Fix Version/s 0.8 [ 12317244 ]
        Resolution Fixed [ 1 ]
        Neha Narkhede made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Jun Rao
            Reporter:
            Jun Rao
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development