-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Make mappings immutable. #15313
Make mappings immutable. #15313
Conversation
@@ -565,18 +585,20 @@ public MultiFields build(FieldMapper.Builder mainFieldBuilder, BuilderContext co | |||
} | |||
|
|||
private final ContentPath.Type pathType; | |||
private volatile ImmutableOpenMap<String, FieldMapper> mappers; | |||
private final ImmutableOpenMap<String, FieldMapper> mappers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love that!
LGTM but I've been reviewing code for about 5 hours now and I'm starting to have some trouble focussing so I'm probably not to be trusted. |
No worries @nik9000, I've seen you been a very active reviewer recently indeed! |
// to be not indexed on another type | ||
// so we use IndexOptions.DOCS here but this will result | ||
// in a mapping merge later | ||
options = IndexOptions.DOCS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the comment...but I still dont' understand...why can't we just throw an error?
+1, I left some minor comments. |
Today mappings are mutable because of two APIs: - Mapper.merge, which expects changes to be performed in-place - IncludeInAll, which allows to change whether values should be put in the `_all` field in place. This commit changes both APIs to return a modified copy instead of modifying in place so that mappings can be immutable. For now, only the type-level object is immutable, but in the future we can imagine making them immutable at the index-level so that mapping updates could be completely atomic at the index level. Close elastic#9365
f8063f5
to
50eeafa
Compare
I just pushed a commit to address review comments. I'll merge soon. |
conflicts.add("Cannot update excludes setting for [_source]"); | ||
} | ||
if (conflicts.isEmpty() == false) { | ||
throw new IllegalArgumentException("Can't merge because of conflicts: " + conflicts); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change also caused later modifications to includes
and excludes
to get denied. Was that intentional?
I wanted to add an exclude for an existing field mapping in the index, so that new documents indexed in the future will no longer store the raw (binary) input data as part of _source
in the index. I will have to rebuild the index now (which will take two days to complete).
I would have expected that includes
and excludes
can be modified after the fact, but the definition will (obviously) only have an impact on newly indexed items.
Today mappings are mutable because of two APIs:
_all
field in place.This commit changes both APIs to return a modified copy instead of modifying in
place so that mappings can be immutable. For now, only the type-level object is
immutable, but in the future we can imagine making them immutable at the
index-level so that mapping updates could be completely atomic at the index
level.
Close #9365