New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host #6682
Conversation
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
.../test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
7aee81d
to
ada5299
Compare
Codecov Report
@@ Coverage Diff @@
## master #6682 +/- ##
===========================================
- Coverage 66.44% 43.47% -22.98%
===========================================
Files 1075 1388 +313
Lines 54773 67508 +12735
Branches 8168 9792 +1624
===========================================
- Hits 36396 29346 -7050
- Misses 15700 35700 +20000
+ Partials 2677 2462 -215
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
...-core/src/main/java/org/apache/pinot/core/data/manager/offline/TableDataManagerProvider.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Pls address all the comments before merging the PR. Thanks!
...src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
Show resolved
Hide resolved
…TimeLeaseExtender for all tables in the host
if (v == null) { | ||
return new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType); | ||
} else { | ||
LOGGER.warn("Lease extender for Table: {} already exists", tableNameWithType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a warning? The method is to getOrCreate()
. Getting the extender is perfectly ok. Please change it to INFO or no log at all.
ServerMetrics serverMetrics, String tableNameWithType) { | ||
return TABLE_TO_LEASE_EXTENDER.compute(tableNameWithType, (k, v) -> { | ||
if (v == null) { | ||
return new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a log here "Creating lease extender for the table {}"
@@ -91,9 +92,12 @@ public synchronized void init(PinotConfiguration config, HelixManager helixManag | |||
Preconditions.checkState(instanceSegmentTarDir.mkdirs()); | |||
} | |||
|
|||
// Initialize segment build time lease extender executor | |||
SegmentBuildTimeLeaseExtender.initExecutor(); | |||
LOGGER.info("Initialized segment build time lease extender executor"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this log to be inside the method initExecutor()
@@ -108,6 +112,8 @@ public synchronized void shutDown() { | |||
for (TableDataManager tableDataManager : _tableDataManagerMap.values()) { | |||
tableDataManager.shutDown(); | |||
} | |||
SegmentBuildTimeLeaseExtender.shutdownExecutor(); | |||
LOGGER.info("Segment build time lease extender executor shut down"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this log to be inside shutDownExecutor()
…aseExtender for all tables in the host (#6682) Co-authored-by: Jiapeng Tao <jiatao@jiatao-mn1.linkedin.biz>
This pr change
SegmentBuildTimeLeaseExtender
from instance level to table level, so removing one table will not accidentally shut down theSegmentBuildTimeLeaseExtender
of other tables.