Lucene - Core
  1. Lucene - Core
  2. LUCENE-5079

allow IndexWriter user to tell if there are uncommitted changes.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      IndexWriter already currently tracks if there are uncommitted changes. We should expose this somehow... perhaps a boolean hasUncommittedChanges()?

      1. LUCENE-5079.patch
        3 kB
        Yonik Seeley
      2. LUCENE-5079.patch
        2 kB
        Yonik Seeley

        Activity

        Hide
        Yonik Seeley added a comment -

        Simple patch adding IndexWriter.hasUncommittedChanges()

        Show
        Yonik Seeley added a comment - Simple patch adding IndexWriter.hasUncommittedChanges()
        Hide
        Michael McCandless added a comment -

        +1

        I think the test case comment "// this will be true because a commit will create a new segment" should instead say "// this will be true because a commit will create an empty index"?

        Show
        Michael McCandless added a comment - +1 I think the test case comment "// this will be true because a commit will create a new segment" should instead say "// this will be true because a commit will create an empty index"?
        Hide
        Uwe Schindler added a comment -

        Do we need synchonization here? - I have not looked at the code, just want to be sure!

        Show
        Uwe Schindler added a comment - Do we need synchonization here? - I have not looked at the code, just want to be sure!
        Hide
        Yonik Seeley added a comment - - edited

        Do we need synchonization here?

        Perhaps just for the fact that the JMM doesn't guarantee we see the non-volatile long atomically?
        We could just make lastCommitChangeCount volatile I guess...

        edit: Actually, it depends on the exact semantics we're looking for. I guess I'll go with synchronized(this) for now for the greater safety.

        Show
        Yonik Seeley added a comment - - edited Do we need synchonization here? Perhaps just for the fact that the JMM doesn't guarantee we see the non-volatile long atomically? We could just make lastCommitChangeCount volatile I guess... edit: Actually, it depends on the exact semantics we're looking for. I guess I'll go with synchronized(this) for now for the greater safety.
        Hide
        Uwe Schindler added a comment -

        edit: Actually, it depends on the exact semantics we're looking for. I guess I'll go with synchronized(this) for now for the greater safety.

        That is the only correct way to do it! lastCommitChangeCount is always guarded by synchronized(this) in the whole code. Any synchronization in this method is unlikely to be a bottleneck, as you wonÄt call this method all the time

        Show
        Uwe Schindler added a comment - edit: Actually, it depends on the exact semantics we're looking for. I guess I'll go with synchronized(this) for now for the greater safety. That is the only correct way to do it! lastCommitChangeCount is always guarded by synchronized(this) in the whole code. Any synchronization in this method is unlikely to be a bottleneck, as you wonÄt call this method all the time
        Hide
        Uwe Schindler added a comment -

        Alternatively you can make the whole method synchronized. sync(this) is just more explicit and goes in line with the other uses of lastCommitChangeCount

        Show
        Uwe Schindler added a comment - Alternatively you can make the whole method synchronized. sync(this) is just more explicit and goes in line with the other uses of lastCommitChangeCount
        Hide
        Yonik Seeley added a comment -

        Actually, I'm leaning again toward a fast version since it might be nice to have on a GUI display and it's not clear how long IW may spend holding the lock on this.

        After further code review, using a volatile seems safe.

        Show
        Yonik Seeley added a comment - Actually, I'm leaning again toward a fast version since it might be nice to have on a GUI display and it's not clear how long IW may spend holding the lock on this. After further code review, using a volatile seems safe.
        Hide
        Yonik Seeley added a comment -

        Any synchronization in this method is unlikely to be a bottleneck

        It's unclear to me if there is any IO that goes on with the lock on "this" held...

        Show
        Yonik Seeley added a comment - Any synchronization in this method is unlikely to be a bottleneck It's unclear to me if there is any IO that goes on with the lock on "this" held...
        Hide
        Michael McCandless added a comment -

        I think volatile is best ... there is IO that happens when "this" is locked, e.g. when applying deletes.

        Show
        Michael McCandless added a comment - I think volatile is best ... there is IO that happens when "this" is locked, e.g. when applying deletes.
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Unassigned
            Reporter:
            Yonik Seeley
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development