Skip to content
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

Put mapping operations must update metadata of all types. #16264

Merged

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Jan 27, 2016

Today put mapping operations only update metadata of the type that is being
modified, which is not enough since some modifications may have side-effects
on other types.

Closes #16239

@jpountz
Copy link
Contributor Author

jpountz commented Feb 8, 2016

@rjernst could you give it a look?

@@ -320,28 +319,24 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt

}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Is this else really necessary anymore? Looks like just some logging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is logging only indeed. But whether or not to log things tends to be a bit controversial so I'd rather do it in another change if we want to remove it

Copy link
Member

Choose a reason for hiding this comment

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

Sure, it was just an observation. :)

@rjernst
Copy link
Member

rjernst commented Feb 8, 2016

LGTM

Today put mapping operations only update metadata of the type that is being
modified, which is not enough since some modifications may have side-effects
on other types.

Closes elastic#16239
@jpountz jpountz force-pushed the fix/update_mapping_meta_on_all_types branch from c57ab2e to 4ab7a18 Compare February 10, 2016 09:38
jpountz added a commit that referenced this pull request Feb 10, 2016
…_types

Put mapping operations must update metadata of all types.
@jpountz jpountz merged commit a5e5211 into elastic:master Feb 10, 2016
@rjernst
Copy link
Member

rjernst commented Feb 10, 2016

@jpountz This is a pretty bad bug for mapping validation, and the fix is relatively simple. Should we put it into 2.2.1?

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2016

+1 to 2.2.1 and also port it to 2.1 in case we do a bugfix release there too?

@jpountz
Copy link
Contributor Author

jpountz commented Feb 11, 2016

The problem is that 2.2 has another bug: when you update a mapping with update_all_types=true, it will not refresh the mapping source of other types (we had this bug since 2.0 and it was fixed as a side-effect of #15539) so this fix alone won't help, we would also need to fix mapping updates to refresh the source of all types. If the consensus is that we should do it, I will do it, but these are big changes so I'm not comfortable pushing them to a bugfix release.

@jpountz jpountz deleted the fix/update_mapping_meta_on_all_types branch February 11, 2016 13:03
@travisjeffery
Copy link

Is there a workaround for 2.2? This bug is a really bad bug to leave like it is.

@rjernst
Copy link
Member

rjernst commented Mar 3, 2016

@travisjeffery the workaround would be to explicitly send updates to every type that contains the field you are changing.

@clintongormley
Copy link
Contributor

@rjernst @travisjeffery there is no way to do that. The update-mapping API only works on a single type at a time. The only time you can set the mapping for multiple types is at index creation.

@rjernst
Copy link
Member

rjernst commented Mar 3, 2016

@clintongormley It does work. Updating them in one internal method call doesn't matter. We just need to get the mappings in memory back into the correct state.

DELETE test_index

PUT test_index
{
  "mappings": {
    "type1": {
      "properties": {
        "field": {
          "type": "string"
        }
      }
    },
    "type2": {
      "properties": {
        "field": {
          "type": "string"
        }
      }
    }
  }
}

PUT test_index/type1/_mapping?update_all_types=true
{
  "properties": {
    "field": {
      "type": "string",
      "search_analyzer": "keyword",
      "analyzer": "default"
    }
  }
}

GET test_index/_mapping?pretty
{
  "test_index" : {
    "mappings" : {
      "type1" : {
        "properties" : {
          "field" : {
            "type" : "string",
            "analyzer" : "default",
            "search_analyzer" : "keyword"
          }
        }
      },
      "type2" : {
        "properties" : {
          "field" : {
            "type" : "string"
          }
        }
      }
    }
  }
}

PUT test_index/type2/_mapping?update_all_types=true
{
  "properties": {
    "field": {
      "type": "string",
      "search_analyzer": "keyword",
      "analyzer": "default"
    }
  }
}

GET test_index/_mapping?pretty
{
  "test_index" : {
    "mappings" : {
      "type1" : {
        "properties" : {
          "field" : {
            "type" : "string",
            "analyzer" : "default",
            "search_analyzer" : "keyword"
          }
        }
      },
      "type2" : {
        "properties" : {
          "field" : {
            "type" : "string",
            "analyzer" : "default",
            "search_analyzer" : "keyword"
          }
        }
      }
    }
  }
}

@clintongormley
Copy link
Contributor

Ah I see what you mean: using multiple calls to update mapping, ok

@fengzhi
Copy link

fengzhi commented May 16, 2017

es 5.2 , I have many types. How can I add my new field to each type . I tryed put mapping with update_all_types but it does not actually update all types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types v2.3.0 v5.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants