Skip to content

Add capability to remove keys from MemoryMapState #48

Merged
asfgit merged 1 commit intoapache:masterfrom
nathanmarz:removable-memory-map-state
Mar 25, 2014
Merged

Add capability to remove keys from MemoryMapState #48
asfgit merged 1 commit intoapache:masterfrom
nathanmarz:removable-memory-map-state

Conversation

@nathanmarz
Copy link
Contributor

The implementation maintains proper opaque semantics, so if the batch is retried the keys will still be there. Keys are officially removed once the next batch starts. This also fixes a bug in OpaqueMap where get after put/update in the same batch wasn't working properly.

@nathanmarz nathanmarz changed the title Add capability to remove keys from MemorymapState Add capability to remove keys from MemoryMapState Mar 17, 2014
@ptgoetz
Copy link
Member

ptgoetz commented Mar 20, 2014

+1

@revans2
Copy link
Contributor

revans2 commented Mar 24, 2014

I don't understand the internals of trident quite as well as the rest of storm. I am a bit confused by the change to OpaqueMap.multiGet. I can see that it is a good performance improvement, but I don't see how it relates to multiRemove. I am fine with putting it in, and the tests look fine so I am +1 on the fix, but I would like to be sure that I understood the code correctly.

@nathanmarz
Copy link
Contributor Author

Trident stores the batch id with any state it stores. It then uses this batch id to decide how to do updates. Opaque maps will update using the "prev val" if the batch id is the same, otherwise it will update using the current val (due to the nature of opaque semantics). Now the problem comes in when you do multiple updates to the same state in the same batch. In this case, the batch id will be the same on the second update, but you should update using the curr val instead of the prev val.

The code was correct on updates but not on gets. If the value was updated in the batch the current val should always be returned, otherwise the standard opaque semantics should be done. Whether a value has had updates applied to it or not is detected with the "CachedBatchReadsMap".

@revans2
Copy link
Contributor

revans2 commented Mar 24, 2014

I get it now. Thanks for the explanation. +1

@asfgit asfgit merged commit 6959401 into apache:master Mar 25, 2014
knusbaum pushed a commit to knusbaum/incubator-storm that referenced this pull request Feb 11, 2015
arunmahadevan pushed a commit to arunmahadevan/storm that referenced this pull request Apr 7, 2016
Bug 46683 - Storm HDFS bolt should ack after tuples are synced
leusonmario pushed a commit to leusonmario/storm that referenced this pull request Aug 20, 2018
Vagrant  storm/mesos cluster setup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants