averma21 commented on a change in pull request #383:
URL: https://github.com/apache/jackrabbit-oak/pull/383#discussion_r727798177
##########
File path:
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/StoreAndSortStrategy.java
##########
@@ -62,8 +63,12 @@ public StoreAndSortStrategy(Iterable<NodeStateEntry>
nodeStates, PathElementComp
@Override
public File createSortedStoreFile() throws IOException {
- File storeFile = writeToStore(storeDir, getStoreFileName());
- return sortStoreFile(storeFile);
+ try {
+ File storeFile = writeToStore(storeDir, getStoreFileName());
+ return sortStoreFile(storeFile);
+ } finally {
+ nodeStates.close();
Review comment:
yes, this looks weird since this class is not the owner of nodeStates.
This has been done to prevent OOM for multi threaded download. We need to keep
on closing the nodeStates as the download tasks keep on finishing. This is
mainly for TraverseAndSortTask (tasks created for parallel download) but due to
code flow same pattern had to be followed here.
We could improve this but some more refactoring would be needed.
##########
File path:
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/StoreAndSortStrategy.java
##########
@@ -62,8 +63,12 @@ public StoreAndSortStrategy(Iterable<NodeStateEntry>
nodeStates, PathElementComp
@Override
public File createSortedStoreFile() throws IOException {
- File storeFile = writeToStore(storeDir, getStoreFileName());
- return sortStoreFile(storeFile);
+ try {
+ File storeFile = writeToStore(storeDir, getStoreFileName());
+ return sortStoreFile(storeFile);
+ } finally {
+ nodeStates.close();
Review comment:
Oh I see you mean that particular method - createSortedStoreFile.
I will see if we can add close method as you suggested.
##########
File path:
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/TraverseAndSortTask.java
##########
@@ -172,14 +173,24 @@ private boolean registerWithMemoryManager() {
log.info("Completed task {}", taskID);
completedTasks.add(taskID);
DirectoryHelper.markCompleted(sortWorkDir);
- if (MemoryManager.Type.JMX_BASED.equals(memoryManager.getType())) {
- memoryManager.deregisterClient(registrationID);
- }
return sortedFiles;
} catch (IOException e) {
log.error(taskID + " could not complete download ", e);
} finally {
phaser.arriveAndDeregister();
+ log.info("{} entered finally block.", taskID);
+ if (dataDumpNotifyingPhaser != null) {
+ log.info("{} Data dump phaser not null after task completion.
Notifying memory listener.", taskID);
+ dataDumpNotifyingPhaser.arriveAndDeregister();
+ }
+ if (MemoryManager.Type.JMX_BASED.equals(memoryManager.getType())) {
+ memoryManager.deregisterClient(registrationID);
+ }
+ try {
+ nodeStates.close();
Review comment:
see
https://github.com/apache/jackrabbit-oak/pull/383#discussion_r727798177
##########
File path:
oak-run-commons/src/test/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/FlatFileStoreTest.java
##########
@@ -294,7 +295,7 @@ public NodeStateEntryTraverser create(LastModifiedRange
range) {
null, range) {
@Override
public @NotNull Iterator<NodeStateEntry> iterator() {
- Map<String, Long> times = new HashMap<>();
+ Map<String, Long> times = new LinkedHashMap<>(); // should
be sorted in increasing order of value i.e. lastModificationTime
Review comment:
here sorting is needed in increasing order of value.
##########
File path:
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/flatfile/DefaultMemoryManager.java
##########
@@ -209,11 +211,15 @@ public void handleNotification(Notification notification,
.getType()
.equals(MemoryNotificationInfo.MEMORY_COLLECTION_THRESHOLD_EXCEEDED)) {
if (sufficientMemory.get()) {
- CompositeData cd = (CompositeData) notification
- .getUserData();
- MemoryNotificationInfo info = MemoryNotificationInfo
- .from(cd);
- checkMemory(info.getUsage());
+ synchronized (sufficientMemory) {
+ if (sufficientMemory.get()) {
Review comment:
As per the current code, we don't want to call checkMemory again, if
sufficientMemory is already false, hence this approach.
--
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]
