DRILL-5270: Improve loading of profiles listing in the WebUI#755

Closed

kkhatua wants to merge 4 commits intoapache:masterfrom

kkhatua:DRILL-5270

Contributor

kkhatua commented Feb 22, 2017

Using Hadoop API to filter and reduce profile list load time
Using an in-memory treeSet-based cache, maintain the list of most recent
profiles.

Contributor Author

kkhatua commented Feb 22, 2017

A summary of the performance is available in this comment on the JIRA (DRILL-5270)

Contributor Author

kkhatua commented Feb 22, 2017

For 8266 profiles, when measured from Chrome browser's Network tool:

Load First Time: 2.43s 
Load Second Time (no new profiles): 829ms

kkhatua force-pushed the DRILL-5270 branch 3 times, most recently from fc15c30 to f7ad29b Compare

April 22, 2017 00:12

Contributor Author

kkhatua commented Apr 22, 2017

@sudheeshkatkam Can you please review the PR?

arina-ielchiieva requested changes

View reviewed changes

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/DrillSysFilePathFilter.java

		@@ -0,0 +1,53 @@
		/**

Member

arina-ielchiieva Feb 27, 2018

Please use comment for the header, not javadoc.

Contributor Author

kkhatua Mar 2, 2018

OK. Will fix this.

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java Outdated

+                //Provides a threshold above which we report the time to load
+                private static final long LISTTIME_THRESHOLD_MSEC = 2000L;
+                private static final int DrillSysFileExtSize = DRILL_SYS_FILE_SUFFIX.length();

Member

arina-ielchiieva Feb 27, 2018

DrillSysFileExtSize -> drillSysFileExtSize

Contributor Author

kkhatua Mar 2, 2018

I wanted to treat this like a constant, but this makes it confusing as a Class name

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java Outdated

                 private final AutoCloseableLock writeLock = new AutoCloseableLock(readWriteLock.writeLock());
+                //Provides a threshold above which we report the time to load
+                private static final long LISTTIME_THRESHOLD_MSEC = 2000L;

Member

arina-ielchiieva Feb 27, 2018

LISTTIME_THRESHOLD_MSEC -> LIST_TIME_THRESHOLD_MSEC

Contributor Author

kkhatua Mar 2, 2018

OK. Will fix this.

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java Outdated

+                  try {
+                    currBasePathModified = fs.getFileStatus(basePath).getModificationTime();
+                  } catch (IOException ioexcp) {
+                    ioexcp.printStackTrace();

Member

arina-ielchiieva Feb 27, 2018

Please do not use printStackTrace()

Contributor Author

kkhatua Mar 2, 2018

Will publish a log message and return an empty iterator for now. Not sure how to bubble up an error to the UI. I'll take a look at how we do so for profile deserialization as a guide

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java

+                    ioexcp.printStackTrace();
+                  }
+                  //Acquiring lock to avoid reloading for request coming in before completion of profile read

Member

arina-ielchiieva Feb 27, 2018

Before reading lock acquirement was enough, with your changes you modify class fields. Since many threads can access this method, you'll end up with raise conditions, also class fields can be cached by threads as well... I think design here should be reconsidered.
Guava library has several cache implementations. Can we leverage any of them instead of using tree set?

Pinging @vlad since he is working on DRILL-6053 which intends to make changes in the same class to avoid excessive locking to be aware of intended changes.

Contributor Author

kkhatua Mar 2, 2018 •

edited

Loading

I'll provide the explanation below on my design choices. Is there a way I can prevent the threads from caching the fields?

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java Outdated

+                /**
+                 * Add profile name to a TreeSet
+                 * @param profileName

Member

arina-ielchiieva Feb 27, 2018

Please do not leave @param, @return without description. IDE usually highlights them, asking to add description.

Contributor Author

kkhatua Mar 2, 2018

OK. Will fix this. Eclipse didn't pop it up for me.

exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/DrillSysFilePathFilter.java

+              /**
+               * Filter for Drill System Files
+               */
+              public class DrillSysFilePathFilter implements PathFilter {

Member

arina-ielchiieva Feb 27, 2018

Please consider using FileSystemUtil which help to create filters. Passing custom filter is also possible.

Contributor Author

kkhatua Mar 2, 2018

Ok. I was thinking of using

List<FileStatus> fileStatuses = DrillFileSystemUtil.listFiles(fs, basePath, false, sysFileSuffixFilter);

Contributor Author

kkhatua commented Mar 2, 2018

@arina-ielchiieva I need to rebase this on top of the latest master considering it was originally based on nearly a year old code. When ready, i'll create a new PR or push to this one. Let me know which one works.

Contributor Author

kkhatua commented Mar 2, 2018 •

edited

Loading

The choice for a TreeSet is to basically use a binary structure that keeps the (maximum permitted) profiles sorted and in memory.

When Drill detect changes,
(Refer https://github.com/kkhatua/drill/blob/f7ad29b9a322bb215d16b3c3b9a2bfc40abfc1ed/exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/LocalPersistentStore.java#L146)
it will fetch all the available profiles in the PStore and reconstruct the tree (since the order of the profiles returned by the FileSystem is not guaranteed).

I tried using the PathFilter to fetch only new profiles, but the cost of the FileSystem fetching only new profiles, versus the entire list is the same! Also, there is the possibility that some profiles might have been deleted as new ones were added, so a full reconstruction would take care of that scenario as well.

To evict, as I construct the TreeSet, I simply pop the oldest (by filename) entry. The Guava cache options don't seem to provide a way to define the basis on which to evict entries.

I believe, @vrozov's work on DRILL-6053 is to address locking during writes specifically. The lock I used (and need) is for reads to ensure that multiple requests don't trigger an expensive FileSystem call for the same state of the PStore.
e.g. consider T# as timestamps

currBasePathModified = T0
ThreadA requests at t=T1 and issues a read-lock
ThreadB requests at t=T2 but is waiting for read-lock

If the tree exists and no change is detected, ThreadA will use the TreeSet contents and resume by releasing the lock.

If the TreeSet exists and a change is detected, ThreadA will reconstruct the TreeSet before using its contents and it will update lastBasePathModified, before releasing the lock.

When ThreadB gets the read-lock, it discovers that during the wait, the TreeSet was already updated. So, in terms of t=T2, this is the most recent snapshot, so it proceeds to use the treeSet's contents rather than reconstruct. The reconstruction will be deferred until the next request comes in.

We're using the lastBasePathModified as a way to provide a pseudo-versioned access to the list. That means if there are more profiles added after ThreadB was waiting for the read-lock, it will not trigger the FileSystem call right away.

Member

vrozov commented Mar 2, 2018

The read locks are not exclusive (single writer/multiple readers). To achieve the required functionality you need to introduce a different lock and use write (or exclusive) lock.
The choice for TreeSet is not obvious. What are the most common operations performed on the collection? Do you optimize for get, put or collection construction?

@arina-ielchiieva my github id is vrozov.

Contributor Author

kkhatua commented Mar 2, 2018

Thanks, @vrozov. I'll make use of a separate lock for read-only purpose in case of #1.
For #2, I need to construct a size-limited ordered set from a list of unordered elements.
In this case, the elements (i.e. profiles) need to be ordered by file-name, which is a 1:1 mapping function of the start time epoch for the query.
So, I need to be able to add to such a datastructure in O(log(n)) time, remove in O(1) and iterate through it in sequence. So, my puts are the most expensive operation.

kkhatua force-pushed the DRILL-5270 branch from f7ad29b to c59977e Compare

March 12, 2018 17:32

Contributor Author

kkhatua commented Mar 14, 2018

Holding off to do a rebase once @vrozov 's PR #1163 (DRILL-6053) goes into Apache.

Kunal Khatua added 3 commits

March 14, 2018 23:39


          DRILL-5270: Improve loading of profiles listing in the WebUI

Using Hadoop API to filter and reduce profile list load time, with a synchronization lock to avoid reloading from the DFS
Using an in-memory treeSet-based cache, maintain the list of most recent
profiles.
Reload of the profiles is done in the event of any of the following states changing:
1. Modification Time of profile dir
2. Number of profiles in the profile dir
3. Number of profiles requested exceeds existing the currently available list


          Using exclusive ReentrantLock

e5a03cb


          Additional Updates

dc29094

1. Clocking total time to serialize the profiles.
2. Locking the profileSet until the transformed iterator is returned.
3. Converted transform function and trailing string to one-time init & constant respectively. (Let JVM optimize)

kkhatua force-pushed the DRILL-5270 branch from db8dd57 to dc29094 Compare

March 15, 2018 06:40


          Added Auto-Archiving

864cacc

kkhatua mentioned this pull request

DRILL-5270: Improve loading of profiles listing in the WebUI #1250

Closed

Contributor Author

kkhatua commented May 4, 2018

Closing this PR in favor of #1250

kkhatua closed this

kkhatua mentioned this pull request

DRILL-5270: Improve loading of profiles listing in the WebUI #1654

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet