Skip to content

Conversation

@idodeclare
Copy link
Contributor

Hello,

Please consider for integration this patch to support running ctags for historical revisions.

  • Add webappCtags configuration flag, with --webappCtags switch, to indicate if the webapp is eligible to run ctags.
  • Add Repository.getHistoryGet() override to allow sub-classes to override to avoid a full in-memory version; and override for Git and Mercurial.
  • Revise BoundedBlockingObjectPool as LIFO-to-FIFO so the webapp does not start extraneous
    instances; Indexer switches to FIFO performance when the queue pool is emptied.
  • make IndexerParallelizer a lazy property of RuntimeEnvironment, and make the executors of IndexerParallelizer also lazy properties. Move the history-related executors into IndexerParallelizer so the lifecycle of all indexing/history executors are controlled in the same class.

Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

please dont use array for out param, think about something better, object reference or returning a pair or smth else

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't get why this is called bounce. I'd prefer clear or close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"bounce" has a long usage in operations for the actions to bring down services and bring them up again immediately (or arrange that they come up on the next request).

IndexerParallelizer is an operations class that helps to coordinate the parallelization of work needed by OpenGrok, so I thought bounce() was apropos. After calling the method, the instance is ready and able to do more work (as demanded by OpenGrok test classes). I don't agree that close() or clear() is more informative.

@coveralls
Copy link

coveralls commented Dec 10, 2018

Pull Request Test Coverage Report for Build 3992

  • 160 of 254 (62.99%) changed or added relevant lines in 24 files are covered.
  • 6 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.03%) to 73.345%

Changes Missing Coverage Covered Lines Changed/Added Lines %
opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/RuntimeEnvironment.java 6 7 85.71%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/BazaarRepository.java 2 3 66.67%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/RCSRepository.java 3 4 75.0%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/SCCSRepository.java 3 4 75.0%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/GitRepository.java 10 12 83.33%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java 9 11 81.82%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/Repository.java 12 14 85.71%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/BitKeeperRepository.java 4 7 57.14%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/HistoryGuru.java 2 5 40.0%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/SubversionRepository.java 2 6 33.33%
Files with Coverage Reduction New Missed Lines %
opengrok-indexer/src/main/java/org/opengrok/indexer/util/BoundedBlockingObjectPool.java 1 65.08%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/MercurialRepository.java 1 82.83%
opengrok-indexer/src/main/java/org/opengrok/indexer/history/RepoRepository.java 1 26.67%
opengrok-indexer/src/main/java/org/opengrok/indexer/index/IndexDatabase.java 3 54.3%
Totals Coverage Status
Change from base Build 3990: 0.03%
Covered Lines: 32863
Relevant Lines: 44806

💛 - Coveralls

Copy link
Contributor

Choose a reason for hiding this comment

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

does it need to be non-static? It doesn't need any state from the enclosing class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm what does it matter for a class with no methods or initializers?

Copy link
Contributor

Choose a reason for hiding this comment

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

It contains unnecessary reference to the enclosing class

@vladak
Copy link
Member

vladak commented Jan 4, 2019

Looks like after the recent commits this is good to go.

@vladak vladak self-assigned this Jan 4, 2019
- Add webappCtags configuration flag, with
  --webappCtags switch, to indicate if the webapp
  is eligible to run ctags.
- Add Repository.getHistoryGet() override to
  allow sub-classes to override to avoid a full
  in-memory version; and override for Git and
  Mercurial.
- Revise BoundedBlockingObjectPool as LIFO-to-FIFO
  so the webapp does not start extraneous
  instances; Indexer switches to FIFO performance
  when the queue pool is emptied.
- make IndexerParallelizer a lazy property of
  RuntimeEnvironment, and make the executors of
  IndexerParallelizer also lazy properties. Move
  the history-related executors into
  IndexerParallelizer so the lifecycle of all
  indexing/history executors are controlled in
  the same class.
@idodeclare idodeclare force-pushed the feature/history_ctags branch from 688d00a to e861a90 Compare February 8, 2019 00:21
@tarzanek tarzanek added this to the 1.2 milestone Feb 8, 2019
@tarzanek
Copy link
Contributor

tarzanek commented Feb 8, 2019

so let's merge

@tarzanek tarzanek merged commit 36334d3 into oracle:master Feb 8, 2019
@idodeclare idodeclare deleted the feature/history_ctags branch February 8, 2019 18:35
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.

5 participants