Skip to content

KAFKA-9375 Add thread names to kafka connect#7901

Merged
mimaison merged 3 commits intoapache:trunkfrom
cryptoe:apache/trunk2
Jan 31, 2020
Merged

KAFKA-9375 Add thread names to kafka connect#7901
mimaison merged 3 commits intoapache:trunkfrom
cryptoe:apache/trunk2

Conversation

@cryptoe
Copy link
Copy Markdown
Contributor

@cryptoe cryptoe commented Jan 7, 2020

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

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

@cryptoe
Copy link
Copy Markdown
Contributor Author

cryptoe commented Jan 7, 2020

@ryannedolan / @mimaison Can you please review this too ?

gcsaba2
gcsaba2 reviewed Jan 10, 2020
View reviewed changes
Comment thread ...untime/src/test/java/org/apache/kafka/connect/runtime/distributed/DistributedHerderTest.java Outdated
Copy link
Copy Markdown

@gcsaba2 gcsaba2 Jan 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is going to work? I don't see Guava in dependencies.gradle and can't find any com.google packages anywhere in the code.

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad . Seems my local intellij needed some cleanup. Thanks for the catch.

@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jan 13, 2020

Thanks for the PR, I'll try to review it later this week.

mimaison
mimaison requested changes Jan 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! It's a nice improvement. I've left a few comments

Comment thread clients/src/test/java/org/apache/kafka/common/utils/ThreadUtilsTest.java Outdated

@Test
public void testThreadNameWithoutNumberNoDemon() {
Assert.assertEquals(ThreadUtils.createThreadFactory(THREAD_NAME, false).
Copy link
Copy Markdown
Member

@mimaison mimaison Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all assert method, the first argument is the "expected" value while the second one is the "actual" value. Can you swap them in this file to match that definition?

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acked

Comment thread clients/src/test/java/org/apache/kafka/common/utils/ThreadUtilsTest.java Outdated
daemonThread.join();
} catch (InterruptedException e) {
// can be ignored
e.printStackTrace();
Copy link
Copy Markdown
Member

@mimaison mimaison Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we don't need to print the stack trace if we can ignore it

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Will do that

Comment thread clients/src/test/java/org/apache/kafka/common/utils/ThreadUtilsTest.java
public void testThreadNameWithNumberDemon() {
ThreadFactory localThreadFactory = ThreadUtils.createThreadFactory(THREAD_NAME_WITH_NUMBER, true);
Thread daemonThread1 = localThreadFactory.newThread(EMPTY_RUNNABLE);
Thread daemonThread2 = localThreadFactory.newThread(EMPTY_RUNNABLE);
Copy link
Copy Markdown
Member

@mimaison mimaison Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need daemonThread2 in this test?

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to check if 2 daemonThreads are appropriately numbered.

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/storage/FileOffsetBackingStoreTest.java Outdated
import org.apache.kafka.connect.util.Callback;
import org.easymock.EasyMock;
import org.junit.After;
import org.junit.Assert;
Copy link
Copy Markdown
Member

@mimaison mimaison Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we import static like assertEquals?

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/storage/FileOffsetBackingStoreTest.java Outdated
File tempFile;

private static Map<ByteBuffer, ByteBuffer> firstSet = new HashMap<>();
private static final Runnable EMPTY_RUNNABLE = new Runnable() {
Copy link
Copy Markdown
Member

@mimaison mimaison Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can use the Java 8 syntax: Runnable runnable = () -> {};. This also appears in another file

Copy link
Copy Markdown
Contributor Author

@cryptoe cryptoe Jan 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do that

@cryptoe
Copy link
Copy Markdown
Contributor Author

cryptoe commented Jan 30, 2020

@mimaison Thanks for looking into this. I have updated the PR based on the review comments.

ryannedolan
ryannedolan approved these changes Jan 30, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ryannedolan ryannedolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thx

mimaison
mimaison approved these changes Jan 31, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mimaison mimaison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jan 31, 2020

retest this please

@mimaison
Copy link
Copy Markdown
Member

mimaison commented Jan 31, 2020

Failures look unrelated, merging

@mimaison mimaison merged commit c8d97c6 into apache:trunk Jan 31, 2020
ijuma added a commit to confluentinc/kafka that referenced this pull request Feb 2, 2020
@ijuma
CONFLUENT: Sync from apache/kafka (1 February 2020)
e52c86c 
Conflicts and/or compiler errors due to the fact that we
temporarily reverted the commit that removes
Scala 2.11 support:

* SslAdminIntegrationTest: keep using JAdminClient,
take upstream changes otherwise.
* ReassignPartitionsClusterTest: keep using
JAdminClient, take upstream changes otherwise.
* KafkaApis: use `asScala.foreach` instead of
`forEach`.

# By Ismael Juma (3) and others
# Via GitHub
* apache-github/trunk: (22 commits)
  KAFKA-9437; Make the Kafka Protocol Friendlier with L7 Proxies [KIP-559] (apache#7994)
  KAFKA-9375: Add names to all Connect threads (apache#7901)
  MINOR: Introduce 2.5-IV0 IBP (apache#8010)
  KAFKA-8503; Add default api timeout to AdminClient (KIP-533) (apache#8011)
  Add retries to release.py script (apache#8021)
  KAFKA-8162: IBM JDK Class not found error when handling SASL (apache#6524)
  MINOR: Add explicit result type in public defs/vals (apache#7993)
  KAFKA-9408: Use StandardCharsets.UTF-8 instead of "UTF-8" (apache#7940)
  KAFKA-9474: Adds 'float64' to the RPC protocol types (apache#8012)
  KAFKA-9360: Allow disabling MM2 heartbeat and checkpoint emissions (apache#7887)
  KAFKA-7658: Add KStream#toTable to the Streams DSL (apache#7985)
  KAFKA-9445: Allow adding changes to allow serving from a specific partition (apache#7984)
  KAFKA-9422: Track the set of topics a connector is using (KIP-558) (apache#8017)
  KAFKA-9040; Add --all option to config command (apache#7607)
  KAFKA-4203: Align broker default for max.message.bytes with Java producer default (apache#4154)
  KAFKA-9426: Use switch instead of chained if/else in OffsetsForLeaderEpochClient (apache#7959)
  KAFKA-9405: Use Map.computeIfAbsent where applicable (apache#7937)
  KAFKA-9026: Use automatic RPC generation in DescribeAcls (apache#7560)
  MINOR: Remove unused fields in StreamsMetricsImpl (apache#7992)
  KAFKA-9460: Enable only TLSv1.2 by default and disable other TLS protocol versions (KIP-553) (apache#7998)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mimaison mimaison mimaison approved these changes

+2 more reviewers

@gcsaba2 gcsaba2 gcsaba2 left review comments

@ryannedolan ryannedolan ryannedolan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cryptoe @mimaison @ryannedolan @gcsaba2