Jackie-Jiang commented on a change in pull request #6165:
URL: https://github.com/apache/incubator-pinot/pull/6165#discussion_r509572456
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1505,10 +1507,19 @@ public void deleteOfflineTable(String tableName) {
LOGGER.info("Deleting table {}: Removed table config", offlineTableName);
// Remove instance partitions
+ final String rawTableName =
TableNameBuilder.extractRawTableName(tableName);
InstancePartitionsUtils.removeInstancePartitions(_propertyStore,
-
InstancePartitionsType.OFFLINE.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableName)));
+
InstancePartitionsType.OFFLINE.getInstancePartitionsName(rawTableName));
LOGGER.info("Deleting table {}: Removed instance partitions",
offlineTableName);
+ // Remove table locker if there
+ if (_tableUpdaterLockMap.containsKey(rawTableName)) {
Review comment:
The key is `offlineTableName` instead of `rawTableName`.
Also, no need to do check and remove here, directly remove should be good
enough
```suggestion
_tableUpdaterLockMap.remove(offlineTableName);
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1641,23 +1652,30 @@ public void addNewSegment(String tableName,
SegmentMetadata segmentMetadata, Str
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap =
Collections
.singletonMap(InstancePartitionsType.OFFLINE, InstancePartitionsUtils
.fetchOrComputeInstancePartitions(_helixZkManager,
offlineTableConfig, InstancePartitionsType.OFFLINE));
- HelixHelper.updateIdealState(_helixZkManager, offlineTableName,
idealState -> {
- assert idealState != null;
- Map<String, Map<String, String>> currentAssignment =
idealState.getRecord().getMapFields();
- if (currentAssignment.containsKey(segmentName)) {
- LOGGER.warn("Segment: {} already exists in the IdealState for table:
{}, do not update", segmentName,
- offlineTableName);
- } else {
- List<String> assignedInstances =
- segmentAssignment.assignSegment(segmentName, currentAssignment,
instancePartitionsMap);
- LOGGER.info("Assigning segment: {} to instances: {} for table: {}",
segmentName, assignedInstances,
- offlineTableName);
- currentAssignment.put(segmentName,
- SegmentAssignmentUtils.getInstanceStateMap(assignedInstances,
SegmentStateModel.ONLINE));
+ if (_tableUpdaterLockMap.get(offlineTableName) == null) {
+ synchronized (_tableUpdaterLockMap) {
+ if (_tableUpdaterLockMap.get(offlineTableName) == null) {
+ _tableUpdaterLockMap.put(offlineTableName, new Object());
+ }
}
- return idealState;
- });
- LOGGER.info("Added segment: {} to IdealState for table: {}",
segmentName, offlineTableName);
+ }
+ synchronized (_tableUpdaterLockMap.get(offlineTableName)) {
Review comment:
```suggestion
synchronized (_tableUpdaterLockMap.computeIfAbsent(offlineTableName, k
-> new Object())) {
```
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1641,23 +1643,30 @@ public void addNewSegment(String tableName,
SegmentMetadata segmentMetadata, Str
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap =
Collections
.singletonMap(InstancePartitionsType.OFFLINE, InstancePartitionsUtils
.fetchOrComputeInstancePartitions(_helixZkManager,
offlineTableConfig, InstancePartitionsType.OFFLINE));
- HelixHelper.updateIdealState(_helixZkManager, offlineTableName,
idealState -> {
- assert idealState != null;
- Map<String, Map<String, String>> currentAssignment =
idealState.getRecord().getMapFields();
- if (currentAssignment.containsKey(segmentName)) {
- LOGGER.warn("Segment: {} already exists in the IdealState for table:
{}, do not update", segmentName,
- offlineTableName);
- } else {
- List<String> assignedInstances =
- segmentAssignment.assignSegment(segmentName, currentAssignment,
instancePartitionsMap);
- LOGGER.info("Assigning segment: {} to instances: {} for table: {}",
segmentName, assignedInstances,
- offlineTableName);
- currentAssignment.put(segmentName,
- SegmentAssignmentUtils.getInstanceStateMap(assignedInstances,
SegmentStateModel.ONLINE));
+ if (_tableUpdaterLockMap.get(offlineTableName) == null) {
+ synchronized (_tableUpdaterLockMap) {
+ if (_tableUpdaterLockMap.get(offlineTableName) == null) {
+ _tableUpdaterLockMap.put(offlineTableName, new Object());
Review comment:
@mcvsubbu A map with less than 10000 entries should be tiny (less than
1MB). Actually the `_segmentCrcMap` and `_lastKnownSegmentMetadataVersionMap`
might cause problem because it stores an entry per segment, but that is out of
the scope of this PR
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -140,6 +140,8 @@
private final Map<String, Map<String, Long>> _segmentCrcMap = new
HashMap<>();
private final Map<String, Map<String, Integer>>
_lastKnownSegmentMetadataVersionMap = new HashMap<>();
+ private final Map<String, Object> _tableUpdaterLockMap = new HashMap<>();
Review comment:
Use `ConcurrentHashMap`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1505,10 +1507,19 @@ public void deleteOfflineTable(String tableName) {
LOGGER.info("Deleting table {}: Removed table config", offlineTableName);
// Remove instance partitions
+ final String rawTableName =
TableNameBuilder.extractRawTableName(tableName);
Review comment:
(nit) we don't use `final` for local variable
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
