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
Update API: Detect noop updates when using doc #6862
Changes from all commits
d74d0b8
b8d87d2
eab5966
0ea5072
c845eaa
f3022ac
16ee1d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,22 +184,40 @@ public static String convertToJson(byte[] data, int offset, int length, boolean | |
/** | ||
* Updates the provided changes into the source. If the key exists in the changes, it overrides the one in source | ||
* unless both are Maps, in which case it recuersively updated it. | ||
* @param source the original map to be updated | ||
* @param changes the changes to update into updated | ||
* @param checkUpdatesAreUnequal should this method check if updates to the same key (that are not both maps) are | ||
* unequal? This is just a .equals check on the objects, but that can take some time on long strings. | ||
* @return true if the source map was modified | ||
*/ | ||
public static void update(Map<String, Object> source, Map<String, Object> changes) { | ||
public static boolean update(Map<String, Object> source, Map<String, Object> changes, boolean checkUpdatesAreUnequal) { | ||
boolean modified = false; | ||
for (Map.Entry<String, Object> changesEntry : changes.entrySet()) { | ||
if (!source.containsKey(changesEntry.getKey())) { | ||
// safe to copy, change does not exist in source | ||
source.put(changesEntry.getKey(), changesEntry.getValue()); | ||
} else { | ||
if (source.get(changesEntry.getKey()) instanceof Map && changesEntry.getValue() instanceof Map) { | ||
// recursive merge maps | ||
update((Map<String, Object>) source.get(changesEntry.getKey()), (Map<String, Object>) changesEntry.getValue()); | ||
} else { | ||
// update the field | ||
source.put(changesEntry.getKey(), changesEntry.getValue()); | ||
} | ||
modified = true; | ||
continue; | ||
} | ||
Object old = source.get(changesEntry.getKey()); | ||
if (old instanceof Map && changesEntry.getValue() instanceof Map) { | ||
// recursive merge maps | ||
modified |= update((Map<String, Object>) source.get(changesEntry.getKey()), | ||
(Map<String, Object>) changesEntry.getValue(), checkUpdatesAreUnequal && !modified); | ||
continue; | ||
} | ||
// update the field | ||
source.put(changesEntry.getKey(), changesEntry.getValue()); | ||
if (modified) { | ||
continue; | ||
} | ||
if (!checkUpdatesAreUnequal || old == null) { | ||
modified = true; | ||
continue; | ||
} | ||
modified = !old.equals(changesEntry.getValue()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |= ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one I caught when I did the weighting and it looks like reverting that reverted my catch. Or I'm just remembering things. Anyway, I'll fix it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just finishing up - this |= isn't required because we continue above if modified is true so we can be super duper lazy with the .equals method. |
||
} | ||
return modified; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -218,6 +218,11 @@ public void postDeleteByQuery(Engine.DeleteByQuery deleteByQuery) { | |
} | ||
} | ||
|
||
public void noopUpdate(String type) { | ||
totalStats.noopUpdates.inc(); | ||
typeStats(type).noopUpdates.dec(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you mean .inc ? |
||
} | ||
|
||
public void clear() { | ||
totalStats.clear(); | ||
synchronized (this) { | ||
|
@@ -253,11 +258,13 @@ static class StatsHolder { | |
public final MeanMetric deleteMetric = new MeanMetric(); | ||
public final CounterMetric indexCurrent = new CounterMetric(); | ||
public final CounterMetric deleteCurrent = new CounterMetric(); | ||
public final CounterMetric noopUpdates = new CounterMetric(); | ||
|
||
public IndexingStats.Stats stats() { | ||
return new IndexingStats.Stats( | ||
indexMetric.count(), TimeUnit.NANOSECONDS.toMillis(indexMetric.sum()), indexCurrent.count(), | ||
deleteMetric.count(), TimeUnit.NANOSECONDS.toMillis(deleteMetric.sum()), deleteCurrent.count()); | ||
deleteMetric.count(), TimeUnit.NANOSECONDS.toMillis(deleteMetric.sum()), deleteCurrent.count(), | ||
noopUpdates.count()); | ||
} | ||
|
||
public long totalCurrent() { | ||
|
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 think it would be cleaner to always return if the doc has been modified and then to check externally if we want to update a document that has not been modified?
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 thought passing in the flag would be better because it allows us to avoid the equality check if we don't need it. I imagine it'd really only matter for long strings but I also imagine this method receives long strings.
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.
Oh that's a good reason for having such a parameter! Can you add a comment about that?
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.
Absolutely.