remove historycache entries automatically #1675
Conversation
| * Clear entry for single file from history cache. | ||
| * @param file path to the file | ||
| */ | ||
| void clearFile(String file); |
There was a problem hiding this comment.
why isn't this a File instead of a path?
There was a problem hiding this comment.
the path is relative to source root. It cannot be a File object because that would bypass HistoryGuru as a generic interface to concrete history cache backend (remember the days when OpenGrok supported JavaDB history cache next to File history cache).
There was a problem hiding this comment.
ok, then maybe it could be mentioned in the javadoc that it is relative to the src
| @@ -37,6 +38,9 @@ | |||
| import static org.junit.Assert.assertNotNull; | |||
| import static org.junit.Assert.assertNull; | |||
| import static org.junit.Assert.assertTrue; | |||
| import org.opensolaris.opengrok.configuration.Project; | |||
| import org.opensolaris.opengrok.history.HistoryGuru; | |||
| import org.opensolaris.opengrok.history.RepositoryFactory; | |||
There was a problem hiding this comment.
import ordering is wrong in this case I think
There was a problem hiding this comment.
Netbeans does not complain however they should be better grouped.
| @@ -45,22 +49,31 @@ | |||
|
|
|||
| private static TestRepository repository; | |||
|
|
|||
| private final static String ctagsProperty = "org.opensolaris.opengrok.analysis.Ctags"; | |||
There was a problem hiding this comment.
isn't this declared somewhere else? Ctags or elsewhere?
There was a problem hiding this comment.
yep, it's defined in number of places throughout test/ directory. filed #1694
| @@ -629,9 +629,7 @@ public void createCache(Collection<String> repositories) { | |||
| public List<Repository> clearCache(Collection<String> repositories) throws HistoryException { | |||
| List<Repository> repos = getReposFromString(repositories); | |||
| HistoryCache cache = historyCache; | |||
| if (cache == null) { | |||
There was a problem hiding this comment.
why this is not needed anymore?
There was a problem hiding this comment.
I think this can't happen - the historyCache is created in HistoryGuru constructor.
There was a problem hiding this comment.
in the constructor it is in an statement if (env.useHistoryCache()) {, is it ensured that this clearCache is not called when this is not in effect?
There was a problem hiding this comment.
good catch; added bunch of useCache() calls.
| * Clear entry for single file from history cache. | ||
| * @param file path to the file | ||
| */ | ||
| void clearFile(String file); |
There was a problem hiding this comment.
any kind of return value which could be useful for the caller in the future?
There was a problem hiding this comment.
It mimicks the clear() interface. It just logs the error and goes on. The same thing is done for removal of xref files. It can be changed to boolean or int and just ignore the values in IndexDatabase#removeFile() however this seems unnecessary to me.
| private void checkDataExistence(String fileName, boolean shouldExist) { | ||
| RuntimeEnvironment env = RuntimeEnvironment.getInstance(); | ||
|
|
||
| for (String dirName : new String[]{"historycache", IndexDatabase.XREF_DIR}) { |
There was a problem hiding this comment.
FileHistoryCache.HISTORY_DIR?
There was a problem hiding this comment.
I'd rather keep it not exposed. Changing the field to protected does not help.
|
|
||
| // Remove the file and reindex using IndexDatabase directly. | ||
| File file = new File(repository.getSourceRoot(), projectName + File.separator + fileName); | ||
| file.delete(); |
There was a problem hiding this comment.
assertTrue(file.delete(), "file not deleted");
There was a problem hiding this comment.
I don't like having actions done in asserts, makes the code less readable I think.
|
feel free to merge, lgtm |
|
As for "memory hungy" I bet you're referring to |
|
ok to me now |
fixes #767
Also, done some semi-related code cleanup.