Skip to content

KAFKA-7880:Naming worker thread by task id#6275

Merged
rhauch merged 2 commits intoapache:trunkfrom
FreeeeLy:add_worker_thread_name
Mar 5, 2019
Merged

KAFKA-7880:Naming worker thread by task id#6275
rhauch merged 2 commits intoapache:trunkfrom
FreeeeLy:add_worker_thread_name

Conversation

@FreeeeLy
Copy link
Copy Markdown
Contributor

@FreeeeLy FreeeeLy commented Feb 15, 2019

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)

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Feb 18, 2019

@ewencp @rhauch @kkonstantine - Can You or someone from your team please review this PR?

Thanks

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Feb 20, 2019

@mjsax, @guozhangwang - Can You or someone from your team please review this PR?

Thanks.

mageshn
mageshn reviewed Feb 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mageshn mageshn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@FreeeeLy Thanks a lot for the PR. Overall, I believe this will be pretty useful and the changes LGTM

@thatb96
Copy link
Copy Markdown

thatb96 commented Feb 20, 2019 via email

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Feb 20, 2019

retest this please

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Feb 20, 2019

Java11 had one unrelated test failure:

  • kafka.api.SaslSslAdminClientIntegrationTest.testMinimumRequestTimeouts

Java8 had one unrelated test failure:

  • kafka.api.AdminClientIntegrationTest.testMinimumRequestTimeouts

Retest this, please.

@guozhangwang guozhangwang requested a review from rhauch February 20, 2019 17:14
@guozhangwang
Copy link
Copy Markdown
Contributor

guozhangwang commented Feb 20, 2019

cc @rhauch

kkonstantine
kkonstantine approved these changes Feb 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Thanks @FreeeeLy
The changes make sense to me. Even though restoring the thread id is not strictly required, it's a better pattern to follow, so I'm in favor. I have two minor comments, but otherwise LGTM

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java Outdated
Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java Outdated
@kkonstantine
Copy link
Copy Markdown
Contributor

kkonstantine commented Feb 20, 2019

And one more thing I forgot. A brief explanation of the change on the PR that will go in the commit message would be helpful. Thanks again!

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Feb 21, 2019

Java11 passed

Java8 had one unrelated test failure:
kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup

I have retested this several times but they may have some unrelated test failures.
How can I solve this kind of problems?

kkonstantine
kkonstantine approved these changes Feb 23, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kkonstantine kkonstantine left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my comments @FreeeeLy !
LGTM by me. (I wouldn't block this PR if indeed the test failures are unrelated).

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Mar 1, 2019

cc @rhauch

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Mar 1, 2019

Retest this, please.

@FreeeeLy
Copy link
Copy Markdown
Contributor Author

FreeeeLy commented Mar 4, 2019

Retest this, please

rhauch
rhauch approved these changes Mar 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @FreeeeLy!

@rhauch rhauch merged commit 20f701c into apache:trunk Mar 5, 2019
@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Mar 5, 2019

My apologies for not including @kkonstantine and @mageshn on the "Reviewers" comment in the squashed commit message. Thank you both, and I'll not make the same mistake again.

jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
@bob-barrett
Merge remote-tracking branch 'apache/trunk' into ccs-master
14e3d1d 
* apache/trunk:
  KAFKA-7880:Naming worker thread by task id (apache#6275)
  improve some logging statements (apache#6078)
  KAFKA-7312: Change broker port used in testMinimumRequestTimeouts and testForceClose
  KAFKA-7997: Use automatic RPC generation in SaslAuthenticate
  KAFKA-8002; Log dir reassignment stalls if future replica has different segment base offset (apache#6346)
  KAFKA-3522: Add TimestampedKeyValueStore builder/runtime classes (apache#6152)
  HOTFIX: add igore import to streams_upgrade_test
  MINOR: ConsumerNetworkClient does not need to send the remaining requests when the node is not ready (apache#6264)
  KAFKA-7922: Return authorized operations in describe consumer group responses (KIP-430 Part-1)
  KAFKA-7918: Inline generic parameters Pt. III: in-memory window store (apache#6328)
  KAFKA-8012; Ensure partitionStates have not been removed before truncating. (apache#6333)
  KAFKA-8011: Fix for race condition causing concurrent modification exception (apache#6338)
  KAFKA-7912: Support concurrent access in InMemoryKeyValueStore (apache#6336)
  MINOR: Skip quota check when replica is in sync (apache#6344)
  HOTFIX: Change header back to http instead of https to path license header test (apache#6347)
  MINOR: fix release.py script (apache#6317)
  MINOR: Remove types from caching stores (apache#6331)
  MINOR: Improve logging for alter log dirs (apache#6302)
  MINOR: state.cleanup.delay.ms default is 600,000 ms (10 minutes). (apache#6345)
  MINOR: disable Streams system test for broker upgrade/downgrade (apache#6341)
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@FreeeeLy @pengxiaolong
KAFKA-7880:Naming worker thread by task id (apache#6275)
169f3ed 
KAFKA-7880: Name worker thread to include task id

Change Connect's `WorkerTask` to name the thread using the `task-thread-<connectorTaskId>` pattern.

Reviewers: Randall Hauch <rhauch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhauch rhauch rhauch approved these changes

@kkonstantine kkonstantine kkonstantine approved these changes

+1 more reviewer

@mageshn mageshn mageshn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

connect

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@FreeeeLy @thatb96 @guozhangwang @kkonstantine @rhauch @mageshn