KAFKA-6955: Use Java AdminClient in DeleteRecordsCommand by vahidhashemian · Pull Request #5088 · apache/kafka

vahidhashemian · 2018-05-27T21:19:07Z

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

omkreddy · 2018-05-28T17:39:59Z

LGTM

viktorsomogyi

Thanks for the contribution, I left one minor comment but otherwise I think it's good.

viktorsomogyi · 2018-05-30T11:26:28Z

nit: this foreach thing could be simplified in my opinion:

val recordsToDelete = offsetSeq.map(offset => offset._1 -> RecordsToDelete.beforeOffset(offset._2)).toMap.asJava

Thanks! You are right. We can even skip the direct var definition and embed the calculation into the following statement; i.e.

val deleteRecordsResult = adminClient.deleteRecords(offsetSeq.map(offset => (offset._1, RecordsToDelete.beforeOffset(offset._2))).toMap.asJava)

But I'll leave it as you suggested to make it more readable.
I'll push a commit shortly.

ijuma

Thanks for the PR. Shall we remove the relevant methods from the Scala AdminClients along with the tests and also add an upgrade note?

ijuma · 2018-05-31T21:48:18Z

Why not import it as AdminClient? We don't use the other AdminClient anymore so we don't need to rename it.

I wanted to remain consistent with this import in other similar files; e.g. ConfigCommand.scala and DelegationTokenCommand.scala. No preference otherwise, and can follow your suggestion if you still think that's preferred.

The Scala AdminClient is going away and there is no need to have this rename in classes that don't need it.

Sure, I removed the alias. Since the old AdminClient is in the same package, the normal import doesn't seem work; hence the admin.AdminClient reference.

ijuma · 2018-05-31T21:49:31Z

Use { case (a, b) => (choose appropriate variable names) to avoid the ugly _1 and _2

ijuma · 2018-05-31T21:49:53Z

No need for ().

…ests

vahidhashemian

@ijuma, tried to address your comments in the new commit (see one response inline). Also, removed the old method from AdminClient.scala, and as a result updated the corresponding unit tests.

vahidhashemian · 2018-06-01T00:49:33Z

I wanted to remain consistent with this import in other similar files; e.g. ConfigCommand.scala and DelegationTokenCommand.scala. No preference otherwise, and can follow your suggestion if you still think that's preferred.

omkreddy · 2018-06-01T07:14:23Z

@ijuma @vahidhashemian I am not sure that we want to delete methods from scala AdminClient now. we should give users some more time to migrate. KafkaAdminClient.deleteRecords API added in recent 1.1.0 release.
maybe we can drop the AdminClient at once after migrating/deprecating all other methods (consumer group, api versions)?

ijuma · 2018-06-01T07:39:26Z

@omkreddy The Scala AdminClient was never a public API and we removed it from the original KIP (KIP-107) as a result. In addition, we added documentation explaining that it was deprecated when the Java AdminClient was added in the 0.11.0 release. Are you aware of anyone using it?

ijuma

A couple of minor comments, but let's wait for @omkreddy to talk a bit more about his reservations.

ijuma · 2018-06-01T07:42:39Z

    consumer.seekToBeginning(Collections.singletonList(tp))
    assertEquals(0L, consumer.position(tp))

-    client.deleteRecordsBefore(Map((tp, 5L))).get()


Do we have a test like this in AdminClientIntegrationTest? If not, we should move it there. If so, we should just remove this test.

I removed the duplicate delete records tests, and moved the non-duplicate ones over.

ijuma · 2018-06-01T07:43:24Z

        timeout behavior for blocking APIs. In particular, a new <code>poll(Duration)</code> API has been added which
        does not block for dynamic partition assignment. The old <code>poll(long)</code> API has been deprecated and
        will be removed in a future version.</li>
+    <li>The implementation of DeleteRecordsCommand makes use of the new Java-based AdminClient.</li>


I meant that the note should be about removing AdminClient.deleteRecordsBefore.

omkreddy · 2018-06-01T08:43:09Z

@ijuma I am not aware of anyone using deleteRecordsBefore API. But I know few tools/companies using AdminClient methods for consumer group monitoring.

This is similar to AdminUtils.scala, which is not a public API, but used in some projects.
I just felt instead of removing methods, it may be appropriate to drop AdminClient at-once after migrating all the API.

I don't have strong reservations about deleting deleteRecordsBefore from scala client.

ijuma · 2018-06-01T09:09:02Z

The main reason for doing it now is that we are doing a major version bump. And unlike the list groups functionality, deleteRecordsBefore was available for a short period in the Scala AdminClient before it became available in the Java AdminClient.

vahidhashemian

@ijuma, thanks for taking another look. Updated the patch with your suggestions.

vahidhashemian · 2018-06-01T16:12:27Z

    consumer.seekToBeginning(Collections.singletonList(tp))
    assertEquals(0L, consumer.position(tp))

-    client.deleteRecordsBefore(Map((tp, 5L))).get()


I removed the duplicate delete records tests, and moved the non-duplicate ones over.

ijuma

I pushed minor tweaks @vahidhashemian, LGTM with these changes. Are you OK with them?

vahidhashemian · 2018-06-02T00:27:15Z

@ijuma, LGTM. Thanks for the tweaks :)

vahidhashemian · 2018-06-02T00:28:36Z

+      messageCount == 10
+    }, "Expected 10 messages", 3000L)
+
+    client.deleteRecords(Map(topicPartition -> RecordsToDelete.beforeOffset(3L)).asJava).all().get()


A couple more () can be removed here and a few lines below.

ijuma · 2018-06-02T16:33:22Z

retest this please

- Removed internal kafka.admin.AdminClient.deleteRecordsBefore since it's no longer used. - Removed redundant tests and rewrote non redundant ones to use the Java AdminClient. Reviewers: Viktor Somogyi <viktor.somogyi@cloudera.com>, Manikumar Reddy <manikumar.reddy@gmail.com>, Ismael Juma <ismael@juma.me.uk>

viktorsomogyi approved these changes May 30, 2018

View reviewed changes

ijuma reviewed May 31, 2018

View reviewed changes

vahidhashemian added 3 commits May 31, 2018 17:43

KAFKA-6955: Use Java AdminClient in DeleteRecordsCommand

6e71ca1

Minor update

3595e1a

Removed the old method from AdminClient.scala, and updated the unit t…

b25c8f4

…ests

vahidhashemian force-pushed the KAFKA-6955 branch from de1ca81 to b25c8f4 Compare June 1, 2018 02:10

vahidhashemian commented Jun 1, 2018

View reviewed changes

Removed the alias for the new AdminClient class

3beab26

vahidhashemian force-pushed the KAFKA-6955 branch from 3723562 to 3beab26 Compare June 1, 2018 04:42

ijuma reviewed Jun 1, 2018

View reviewed changes

Moved delete records tests to AdminClientIntegrationTest

ce98717

vahidhashemian commented Jun 1, 2018

View reviewed changes

A few tweaks

667939c

ijuma approved these changes Jun 2, 2018

View reviewed changes

vahidhashemian commented Jun 2, 2018

View reviewed changes

ijuma and others added 2 commits June 1, 2018 17:39

Remove ()

b5d6a55

Fixed the expected exception unit test

016200d

vahidhashemian force-pushed the KAFKA-6955 branch from 28788ef to 016200d Compare June 2, 2018 04:12

Minor tweak

28d4e05

ijuma merged commit 341d5db into apache:trunk Jun 2, 2018

vahidhashemian mentioned this pull request Jun 6, 2018

Kafka 6884 consumer group command should use new admin client #5032

Merged

3 tasks

Conversation

vahidhashemian commented May 27, 2018 • edited by ijuma Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

omkreddy commented May 28, 2018

Uh oh!

viktorsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vahidhashemian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkreddy commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Jun 1, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkreddy commented Jun 1, 2018

Uh oh!

ijuma commented Jun 1, 2018

Uh oh!

vahidhashemian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

vahidhashemian commented Jun 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Jun 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vahidhashemian commented May 27, 2018 •

edited by ijuma

Loading

omkreddy commented Jun 1, 2018 •

edited

Loading