Solr
  1. Solr
  2. SOLR-6180

Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.10, 6.0
    • Component/s: Schema and Analysis
    • Labels:
      None

      Description

      Consider the code in FieldResource.java:

      SchemaField newField = oldSchema.newField(fieldName, fieldType, map);
      IndexSchema newSchema = oldSchema.addField(newField, copyFieldNames);
      if (null != newSchema) {
        getSolrCore().setLatestSchema(newSchema);
        success = true;
      } else {
        throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to add field.");
      }
      

      The schema update lock is only held during the call to addField, so there is no guarantee that the schema we are setting later is actually the latest schema. This would rarely happen, but it's possible that between the time we gave up the lock and the time we set the schema that we got a new schema from ZK. We'd end up overwriting the new schema, possibly missing newly added fields.

      1. SchemaLockTest.java
        11 kB
        Gregory Chanan
      2. SOLR-6180.patch
        21 kB
        Steve Rowe
      3. SOLR-6180.patch
        15 kB
        Gregory Chanan

        Activity

        Hide
        Gregory Chanan added a comment -

        Here's a modified version of TestCloudManagedSchemaConcurrent that tests for the issue. To trigger the issue, I put a sleep between the end of the call to addField and the call to setLatestSchema – I doubt it will trigger often without that and so the test itself should just be considered a demonstration not worth committing.

        Show
        Gregory Chanan added a comment - Here's a modified version of TestCloudManagedSchemaConcurrent that tests for the issue. To trigger the issue, I put a sleep between the end of the call to addField and the call to setLatestSchema – I doubt it will trigger often without that and so the test itself should just be considered a demonstration not worth committing.
        Hide
        Gregory Chanan added a comment -

        Here's a patch that exposes getSchemaUpdateLock at the IndexSchema level, and moves synchronizing on the lock from ManagedIndexSchema itself to the callers (this is consistent with how ZkIndexSchemaReader works).

        Show
        Gregory Chanan added a comment - Here's a patch that exposes getSchemaUpdateLock at the IndexSchema level, and moves synchronizing on the lock from ManagedIndexSchema itself to the callers (this is consistent with how ZkIndexSchemaReader works).
        Hide
        Mark Miller added a comment -

        +1, nice catch! Doesn't seem like a bad idea to add the test to me. Even without that 10ms sleep (which we could perhaps randomly do?), I wouldn't be surprised if this doesn't fail sometimes on the jenkins cluster, which is hammering these tests over and over as fast as it can in a variety of slow envs.

        Show
        Mark Miller added a comment - +1, nice catch! Doesn't seem like a bad idea to add the test to me. Even without that 10ms sleep (which we could perhaps randomly do?), I wouldn't be surprised if this doesn't fail sometimes on the jenkins cluster, which is hammering these tests over and over as fast as it can in a variety of slow envs.
        Hide
        Gregory Chanan added a comment -

        thanks for taking a look, Mark.

        FWIW the sleep I put in ("between the end of the call to addField and the call to setLatestSchema"), was random between 1 and 10 seconds. I didn't investigate shorter sleeps to see if it might trigger randomly.

        Show
        Gregory Chanan added a comment - thanks for taking a look, Mark. FWIW the sleep I put in ("between the end of the call to addField and the call to setLatestSchema"), was random between 1 and 10 seconds. I didn't investigate shorter sleeps to see if it might trigger randomly.
        Hide
        Steve Rowe added a comment -

        Looks good.

        The attached patch combines the schema lock test with Gregory's other patch. When I ran the new test without the other patch I got a roughly 50% failure rate when I put a 50ms sleep between addField() and SolrCore.setLatestSchema() in FieldResource.put(), and 100% at 125ms.

        Committing shortly.

        Show
        Steve Rowe added a comment - Looks good. The attached patch combines the schema lock test with Gregory's other patch. When I ran the new test without the other patch I got a roughly 50% failure rate when I put a 50ms sleep between addField() and SolrCore.setLatestSchema() in FieldResource.put() , and 100% at 125ms. Committing shortly.
        Hide
        ASF subversion and git services added a comment -

        Commit 1608646 from Steve Rowe in branch 'dev/trunk'
        [ https://svn.apache.org/r1608646 ]

        SOLR-6180: Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock.

        Show
        ASF subversion and git services added a comment - Commit 1608646 from Steve Rowe in branch 'dev/trunk' [ https://svn.apache.org/r1608646 ] SOLR-6180 : Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock.
        Hide
        ASF subversion and git services added a comment -

        Commit 1608650 from Steve Rowe in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1608650 ]

        SOLR-6180: Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock. (merged trunk r1608646)

        Show
        ASF subversion and git services added a comment - Commit 1608650 from Steve Rowe in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1608650 ] SOLR-6180 : Callers of ManagedIndexSchema mutators should hold the schemaUpdateLock. (merged trunk r1608646)
        Hide
        Steve Rowe added a comment -

        Committed to trunk and branch_4x.

        Thanks Gregory!

        Show
        Steve Rowe added a comment - Committed to trunk and branch_4x. Thanks Gregory!

          People

          • Assignee:
            Steve Rowe
            Reporter:
            Gregory Chanan
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development