Skip to content

various repository methods should use interactive timeout #2108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 21, 2018

Conversation

vladak
Copy link
Member

@vladak vladak commented May 15, 2018

This change introduces interactive timeout that will be used when:

  • setting new configuration (happens e.g. when webapp is starting or when new configuration is set via Message or when Search is done using CLI)
  • getting content of a file in given revision
  • getting repository metadata

The first item basically triggers invalidateRepositories() in HistoryGuru that will in turn call getRepositories(). Thus, all the Repository calls inside need to be made "interactive".

Basically, I've modified all repositories to use Executor as opposed to Runtime or ProcessBuilder directly. I don't like the way how Executor sets up the timeout too much (it should not know about RuntimeEnvironment or unset this.timeout when it is already set. Maybe it should also have getter/setter for timeout.), however that's for next time.

Also, maybe all the repository annotation/tag parsers should be an interface/abstract class however there are subtle difference how these are implemented for various repositories so for the time being I mirrored the style already used by Bitkeeper repository.

After the change there will be 3 types of timeout related behavior:

  • no timeout (full reindex, i.e. no revision specification)
  • command timeout (incremental reindex, i.e. revision specification or number of entries - in case of Subversion)
  • interactive timeout (as described above)

Tested with unreachable Subversion repository + webapp start and also full reindex.

@coveralls
Copy link

coveralls commented May 15, 2018

Pull Request Test Coverage Report for Build 2476

  • 366 of 688 (53.2%) changed or added relevant lines in 41 files are covered.
  • 34 unchanged lines in 15 files lost coverage.
  • Overall coverage decreased (-0.03%) to 73.082%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/org/opensolaris/opengrok/history/PerforceHistoryParser.java 0 1 0.0%
src/org/opensolaris/opengrok/history/BitKeeperTagParser.java 0 1 0.0%
src/org/opensolaris/opengrok/history/CVSRepository.java 9 10 90.0%
src/org/opensolaris/opengrok/history/RepoRepository.java 0 1 0.0%
src/org/opensolaris/opengrok/configuration/Configuration.java 5 7 71.43%
src/org/opensolaris/opengrok/history/BazaarAnnotationParser.java 20 22 90.91%
src/org/opensolaris/opengrok/history/Repository.java 5 7 71.43%
src/org/opensolaris/opengrok/history/MercurialAnnotationParser.java 24 26 92.31%
src/org/opensolaris/opengrok/history/SCCSRepositoryAnnotationParser.java 23 25 92.0%
src/org/opensolaris/opengrok/history/CVSAnnotationParser.java 25 27 92.59%
Files with Coverage Reduction New Missed Lines %
src/org/opensolaris/opengrok/history/FileHistoryCache.java 1 74.09%
src/org/opensolaris/opengrok/history/MercurialRepository.java 1 81.74%
src/org/opensolaris/opengrok/history/RepoRepository.java 1 26.67%
src/org/opensolaris/opengrok/history/SCCSRepository.java 1 79.27%
src/org/opensolaris/opengrok/analysis/AnalyzerGuru.java 2 85.49%
src/org/opensolaris/opengrok/history/HistoryGuru.java 2 73.75%
src/org/opensolaris/opengrok/history/SSCMRepository.java 2 5.56%
src/org/opensolaris/opengrok/configuration/RuntimeEnvironment.java 2 68.87%
src/org/opensolaris/opengrok/history/BazaarRepository.java 2 70.16%
src/org/opensolaris/opengrok/history/ClearCaseRepository.java 3 11.63%
Totals Coverage Status
Change from base Build 2474: -0.03%
Covered Lines: 31075
Relevant Lines: 42521

💛 - Coveralls

Copy link
Contributor

@idodeclare idodeclare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering about interactive in a few places. Otherwise LGTM.

}

@Override
protected void buildTagList(File directory) {
protected void buildTagList(File directory, boolean interactive) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering whether the new interactive parameter should possibly be used in this method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, yes. Also, I've just hit this when testing full reindex on a busy machine where I deliberately set the interactive timeout to 3 seconds.

@@ -295,57 +263,23 @@ boolean hasFileBasedTags() {
* @param directory Directory where we list tags
*/
@Override
protected void buildTagList(File directory) {
protected void buildTagList(File directory, boolean interactive) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if new interactive should be used in this method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -613,70 +561,23 @@ boolean hasFileBasedTags() {
}

@Override
protected void buildTagList(File directory) {
protected void buildTagList(File directory, boolean interactive) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should new interactive be used in the method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@vladak
Copy link
Member Author

vladak commented May 18, 2018

In the end I decided to properly propagate the interactive flag from readConfiguration() so that the interactive mode is not implicit. This resulted in some methods of the Repository abstract class to be redefined.

@vladak
Copy link
Member Author

vladak commented May 18, 2018

As for the Messages handling, specifically ProjectMessage it might be considered interactive since one usually emits them via command line and expects quick response however given this will be actually exposed via REST API, I am leaving them as non-interactive (i.e. the getRepositoriesInDir()/getRepository() calls).

In other words, the definition would be: whatever comes from UI/webapp is considered interactive.

@vladak vladak force-pushed the interactive_timeout branch from 0eddb36 to 9262904 Compare May 21, 2018 08:37
@vladak vladak force-pushed the interactive_timeout branch from 9262904 to 49de4b0 Compare May 21, 2018 08:37
@vladak vladak merged commit b238bfb into oracle:master May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants