krishan1390 commented on code in PR #18381:
URL: https://github.com/apache/pinot/pull/18381#discussion_r3226252619
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/data/manager/SegmentDataManager.java:
##########
@@ -99,8 +99,6 @@ public void offload() {
* The data manager can only be destroyed once.
*/
public void destroy() {
- // NOTE: We want the test to catch the case when destroy is called without
offloading, but not fail the production.
- assert _offloaded.get() : "Cannot destroy segment data manager without
offloading it first";
Review Comment:
I think it makes sense for destroy to call offload first if we want the
segment to be offloaded only after its reference count becomes 0
Also I don't see a reason to complicate clients to do below loop always
```
segmentDataManager.offload();
if (segmentDataManager.decreaseReferenceCount()) {
segmentDataManager.destroy();
}
```
Rather below is simpler
```
if (segmentDataManager.decreaseReferenceCount()) {
segmentDataManager.destroy();
}
```
Thus removing this assertion
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -606,6 +606,70 @@ protected void doOffloadSegment(String segmentName) {
}
}
+ @Override
+ public void deleteSegment(String segmentName)
Review Comment:
This is moved from HelixInstanceDataManager to TableDataManager as this is
the right place
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -1016,7 +1056,10 @@ public void reloadSegment(String segmentName,
IndexLoadingConfig indexLoadingCon
File indexDir = getSegmentDataDir(segmentName, segmentTier,
indexLoadingConfig.getTableConfig());
Lock segmentLock = getSegmentLock(segmentName);
segmentLock.lock();
+ boolean semaphoreAcquired = false;
try {
+ _segmentReloadSemaphore.acquire(segmentName, _logger);
Review Comment:
We are now acquiring the semaphore after taking the segment lock. This is
different from previous reload behaviour where semaphore is taking before the
segment lock. This has a potential side effect where segment lock is held for a
long time if semaphore is throttled.
But this is now consistent with doReplaceSegment() which was already first
taking the lock and then acquiring the same semaphore.
##########
pinot-core/src/test/java/org/apache/pinot/core/data/manager/BaseTableDataManagerAcquireSegmentTest.java:
##########
@@ -73,7 +72,7 @@ public class BaseTableDataManagerAcquireSegmentTest {
private final Set<ImmutableSegment> _allSegments = new HashSet<>();
private final Set<SegmentDataManager> _accessedSegManagers =
ConcurrentHashMap.newKeySet();
private final Set<SegmentDataManager> _allSegManagers =
ConcurrentHashMap.newKeySet();
- private Map<String, ImmutableSegmentDataManager> _internalSegMap;
+ protected Map<String, SegmentDataManager> _internalSegMap;
Review Comment:
These changes are refactoring to enable a more generic and extensible
BaseTableDataManagerTest
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java:
##########
@@ -616,12 +680,7 @@ public void reloadSegment(String segmentName, boolean
forceDownload, String relo
SegmentDataManager segmentDataManager =
_segmentDataManagerMap.get(segmentName);
if (segmentDataManager != null) {
IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig();
- _segmentReloadSemaphore.acquire(segmentName, _logger);
- try {
- reloadSegment(segmentDataManager, indexLoadingConfig, forceDownload);
- } finally {
- _segmentReloadSemaphore.release();
- }
+ reloadSegment(segmentDataManager, indexLoadingConfig, forceDownload);
Review Comment:
moved the sempahore to inside reloadSegment
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
