You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/03/14 08:40:14 UTC
[GitHub] [incubator-pinot] jtao15 opened a new pull request #6682: [WIP]Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
jtao15 opened a new pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682
This pr change `SegmentBuildTimeLeaseExtender` from instance level to table level, so removing one table will not accidentally shut down the `SegmentBuildTimeLeaseExtender` of other tables.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594840078
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,39 +46,58 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
Review comment:
(nit) add final
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,39 +46,58 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
+ }
+
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
+ }
+ }
+
+ public static boolean isExecutorShutdown() {
Review comment:
annotate with VisibleForTestOnly
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/offline/TableDataManagerProvider.java
##########
@@ -45,6 +46,11 @@ public static void init(InstanceDataManagerConfig instanceDataManagerConfig) {
if (maxParallelBuilds > 0) {
_segmentBuildSemaphore = new Semaphore(maxParallelBuilds, true);
}
+ SegmentBuildTimeLeaseExtender.initExecutor();
Review comment:
Suggest moving the `initExecutor()` and `shutdownExecutor()` to `HelixInstanceDataManager`.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,39 +46,58 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
+ }
+
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
+ }
+ }
+
+ public static boolean isExecutorShutdown() {
+ return _executor == null;
}
- public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
+ public static synchronized SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
+ }
+
+ public static synchronized SegmentBuildTimeLeaseExtender getOrCreate(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for Table: {} already exists", tableNameWithType);
} else {
leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
- INSTANCE_TO_LEASE_EXTENDER.put(instanceId, leaseExtender);
+ TABLE_TO_LEASE_EXTENDER.put(tableNameWithType, leaseExtender);
}
return leaseExtender;
}
private SegmentBuildTimeLeaseExtender(String instanceId, ServerMetrics serverMetrics, String tableNameWithType) {
_instanceId = instanceId;
_protocolHandler = new ServerSegmentCompletionProtocolHandler(serverMetrics, tableNameWithType);
- _executor = new ScheduledThreadPoolExecutor(1);
}
public void shutDown() {
- if (_executor != null) {
- _executor.shutdownNow();
- _executor = null;
+ for (Map.Entry<String, Future> entry : _segmentToFutureMap.entrySet()) {
+ Future future = entry.getValue();
+ if (future != null) {
Review comment:
I don't think future can be null here
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,39 +46,58 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
+ }
+
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
+ }
+ }
+
+ public static boolean isExecutorShutdown() {
+ return _executor == null;
}
- public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
+ public static synchronized SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
+ }
+
+ public static synchronized SegmentBuildTimeLeaseExtender getOrCreate(final String instanceId,
Review comment:
You may use `ConcurrentHashMap.compute()` which is atomic. Not very important though because it is not on query path
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594871052
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,39 +46,58 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
+ }
+
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
+ }
+ }
+
+ public static boolean isExecutorShutdown() {
+ return _executor == null;
}
- public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
+ public static synchronized SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
+ }
+
+ public static synchronized SegmentBuildTimeLeaseExtender getOrCreate(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for Table: {} already exists", tableNameWithType);
} else {
leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
- INSTANCE_TO_LEASE_EXTENDER.put(instanceId, leaseExtender);
+ TABLE_TO_LEASE_EXTENDER.put(tableNameWithType, leaseExtender);
}
return leaseExtender;
}
private SegmentBuildTimeLeaseExtender(String instanceId, ServerMetrics serverMetrics, String tableNameWithType) {
_instanceId = instanceId;
_protocolHandler = new ServerSegmentCompletionProtocolHandler(serverMetrics, tableNameWithType);
- _executor = new ScheduledThreadPoolExecutor(1);
}
public void shutDown() {
Review comment:
We should also remove the instance from the `TABLE_TO_LEASE_EXTENDER` map here
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594794451
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -765,6 +768,20 @@ public void testOnlyOneSegmentHoldingTheSemaphoreForParticularPartition()
Assert.assertEquals(secondSegmentDataManager.get().getPartitionGroupConsumerSemaphore().availablePermits(), 1);
}
+ @Test
+ public void testDifferentSegmentBuildTimeLeaseExtenderDestroyedForTables()
Review comment:
Updated the test with better name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#issuecomment-799909360
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=h1) Report
> Merging [#6682](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=desc) (ada5299) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.97%`.
> The diff coverage is `42.85%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=tree)
```diff
@@ 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
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `43.47% <42.85%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...t/broker/broker/BasicAuthAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0Jhc2ljQXV0aEFjY2Vzc0NvbnRyb2xGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| ... and [1414 more](https://codecov.io/gh/apache/incubator-pinot/pull/6682/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=footer). Last update [d877558...ada5299](https://codecov.io/gh/apache/incubator-pinot/pull/6682?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] snleee merged pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
snleee merged pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #6682: [WIP]Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594577905
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
}
public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for {} already exists", tableNameWithType);
} else {
leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
Review comment:
This will create one executor per table. To keep the same overhead of the lease extender, we might want to share the same executor across all tables.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
Review comment:
Make this `ConcurrentHashMap` and remove the `synchronized` on line 60. `HashMap` is not thread-safe on line 57
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on a change in pull request #6682: [WIP]Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
snleee commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594550502
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
Review comment:
Can you change `new HashMap<>(1) to new HashMap<>()`? I think that we specifically built the hashmap with the size 1 because we only kept 1 lease extender per server while we now keep 1 lease extender per table.
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -128,9 +131,9 @@ private TableConfig createTableConfig()
return JsonUtils.stringToObject(_tableConfigJson, TableConfig.class);
}
- private RealtimeTableDataManager createTableDataManager() {
+ private RealtimeTableDataManager createTableDataManager(TableConfig tableConfig) {
Review comment:
Can you add the test for the particular issue that we had if possible?
1. Create 2 data managers for 2 tables.
2. Call the shutdown() from one data manager.
3. Check if the lease extender for 2nd data manager is still alive. (Also double check if this test would fail before the fix)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jackjlli commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594688627
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -765,6 +768,20 @@ public void testOnlyOneSegmentHoldingTheSemaphoreForParticularPartition()
Assert.assertEquals(secondSegmentDataManager.get().getPartitionGroupConsumerSemaphore().availablePermits(), 1);
}
+ @Test
+ public void testDifferentSegmentBuildTimeLeaseExtenderDestroyedForTables()
Review comment:
What's the meaning of this test name? What/How to destroy? Can you add some comments to the test?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
}
public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for {} already exists", tableNameWithType);
Review comment:
`Lease extender for Table: {} already exists`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594793769
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -128,9 +131,9 @@ private TableConfig createTableConfig()
return JsonUtils.stringToObject(_tableConfigJson, TableConfig.class);
}
- private RealtimeTableDataManager createTableDataManager() {
+ private RealtimeTableDataManager createTableDataManager(TableConfig tableConfig) {
Review comment:
Updated the test to check the executor is not removed when tableDataManager is removed. Checked manually that the executor is removed before the fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] snleee commented on pull request #6682: [WIP]Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
snleee commented on pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#issuecomment-799620806
#6671
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594810131
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
Review comment:
Use `ConcurrentHashMap` and remove `synchronized` for `create()` may potential to create multiple lease extender since `create()` is not atomic. I think maybe it's better to keep the `HashMap` and add `synchronized` to the `getLeaseExtender()` also.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594793769
##########
File path: pinot-core/src/test/java/org/apache/pinot/core/data/manager/realtime/LLRealtimeSegmentDataManagerTest.java
##########
@@ -128,9 +131,9 @@ private TableConfig createTableConfig()
return JsonUtils.stringToObject(_tableConfigJson, TableConfig.class);
}
- private RealtimeTableDataManager createTableDataManager() {
+ private RealtimeTableDataManager createTableDataManager(TableConfig tableConfig) {
Review comment:
Updated the test to check the executor is not removed when tableDataManager is removed. Checked manually that the executor will be removed before the fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jtao15 commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594794912
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
}
public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for {} already exists", tableNameWithType);
} else {
leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
Review comment:
Updated with single executor for all tables.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r595315005
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -45,42 +45,63 @@
private static final int EXTRA_TIME_SECONDS = 120;
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
- private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new ConcurrentHashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
+ private final String _tableNameWithType;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
}
- public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
- ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
- if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
- } else {
- leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
- INSTANCE_TO_LEASE_EXTENDER.put(instanceId, leaseExtender);
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
}
- return leaseExtender;
+ }
+
+ @VisibleForTesting
+ public static boolean isExecutorShutdown() {
+ return _executor == null;
+ }
+
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
+ }
+
+ public static SegmentBuildTimeLeaseExtender getOrCreate(final String instanceId,
+ ServerMetrics serverMetrics, String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.compute(tableNameWithType, (k, v) -> {
+ if (v == null) {
+ return new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
+ } else {
+ LOGGER.warn("Lease extender for Table: {} already exists", tableNameWithType);
Review comment:
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.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -45,42 +45,63 @@
private static final int EXTRA_TIME_SECONDS = 120;
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
- private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new ConcurrentHashMap<>();
+ private static ScheduledExecutorService _executor;
- private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
+ private final String _tableNameWithType;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static void initExecutor() {
+ _executor = new ScheduledThreadPoolExecutor(1);
}
- public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
- ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
- if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
- } else {
- leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
- INSTANCE_TO_LEASE_EXTENDER.put(instanceId, leaseExtender);
+ public static void shutdownExecutor() {
+ if (_executor != null) {
+ _executor.shutdownNow();
+ _executor = null;
}
- return leaseExtender;
+ }
+
+ @VisibleForTesting
+ public static boolean isExecutorShutdown() {
+ return _executor == null;
+ }
+
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
+ }
+
+ public static SegmentBuildTimeLeaseExtender getOrCreate(final String instanceId,
+ ServerMetrics serverMetrics, String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.compute(tableNameWithType, (k, v) -> {
+ if (v == null) {
+ return new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
Review comment:
Add a log here "Creating lease extender for the table {}"
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -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");
Review comment:
Move this log to be inside the method initExecutor()
##########
File path: pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -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");
Review comment:
Move this log to be inside shutDownExecutor()
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6682: Fix bug #6671: RealtimeTableDataManager shuts down SegmentBuildTimeLeaseExtender for all tables in the host
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6682:
URL: https://github.com/apache/incubator-pinot/pull/6682#discussion_r594708671
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
}
public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for {} already exists", tableNameWithType);
} else {
leaseExtender = new SegmentBuildTimeLeaseExtender(instanceId, serverMetrics, tableNameWithType);
Review comment:
+1
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
Review comment:
synchronized in line 60 is ok. we check and then insert if it is not there. Alternatively, we can use `computeIfAbsent` to check and insert atomically.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/SegmentBuildTimeLeaseExtender.java
##########
@@ -46,25 +46,25 @@
// Retransmit lease request 10% before lease expires.
private static final int REPEAT_REQUEST_PERIOD_SEC = (EXTRA_TIME_SECONDS * 9 / 10);
private static Logger LOGGER = LoggerFactory.getLogger(SegmentBuildTimeLeaseExtender.class);
- private static final Map<String, SegmentBuildTimeLeaseExtender> INSTANCE_TO_LEASE_EXTENDER = new HashMap<>(1);
+ private static final Map<String, SegmentBuildTimeLeaseExtender> TABLE_TO_LEASE_EXTENDER = new HashMap<>(1);
private ScheduledExecutorService _executor;
private final Map<String, Future> _segmentToFutureMap = new ConcurrentHashMap<>();
private final String _instanceId;
private final ServerSegmentCompletionProtocolHandler _protocolHandler;
- public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String instanceId) {
- return INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ public static SegmentBuildTimeLeaseExtender getLeaseExtender(final String tableNameWithType) {
+ return TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
}
public static synchronized SegmentBuildTimeLeaseExtender create(final String instanceId,
ServerMetrics serverMetrics, String tableNameWithType) {
- SegmentBuildTimeLeaseExtender leaseExtender = INSTANCE_TO_LEASE_EXTENDER.get(instanceId);
+ SegmentBuildTimeLeaseExtender leaseExtender = TABLE_TO_LEASE_EXTENDER.get(tableNameWithType);
if (leaseExtender != null) {
- LOGGER.warn("Instance already exists");
+ LOGGER.warn("Lease extender for {} already exists", tableNameWithType);
Review comment:
Maybe change the name of this method to `getOrCreate()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org