Am Thursday 20 August 2009 16:24:44 schrieb Matthew Toseland: > 9fe7b39acdf5c71a43a32c4787f1cb95637a5458 > ArchivePutHandler.onEncode: > container.activate(BaseManifestPutter.this, 2); > > This is risky! If maps get activated to depth 1 then really bad things > happen in my experience, they need to be activated to depth 2. Activating > to depth 1 then depth 2 does not solve the problem. Also, the map has a > list inside it, i'd expect more activation issues. > > PutHandler constructor: container.store(runningMap) - should probably > specify depth, likewise with two below. > > ContainerPutHandler.onEncode(): on the root container handler, we should > store BaseMetadataPutter.this, not this, no? In the other branch, you > haven't activated putHandlersTransformMap, etc. > > ContainerPutHandler.onSuccess(): activated contaienrPutHandlers?? And you > don't store it when you remove from it. > > ExtPutHandler.onMetadata : activate putHandlersWaitingForMetadata. In > container mode, explicitly store (this), the depth limited store of the > maps won't save the metadata. > > MetaPutHandler.onEncode() : metadata.resolve() : should store it? But also > does it allow to complete upstream resolutions? I guess deferring that to > onSuccess is okay, and may even be safer; in the rest of the code that is > determined by the earlyEncode flag though... and if you want to support > that properly, we might not be finished by the time of onSuccess chaining > to super.onSuccess. > > PutHandler?.onBlockSetFinished : store(collection) with no depth again, > probably bad > > containerMode vs freeformMode - these are mutually exclusive and the only > options? how about a single boolean, or an enum? Hmmm, apparently not, it > changes in getRootContainer(). > > would be nice to document putHandlersTransformMap some comments added at variable decaration in a34fd1794005b71cf7c04c16973aa80fe9f63653 > > start() : why activate putHandlerWaitingForBlockSets? > > makeMetadata() : you are sure this works? I had horrible problems with > individually activating maps inside maps, and imho it follows directly from > the fact that if you activate a map to depth 1 it works but is empty, and > will not fill up when you activate it to depth 2; you must deactivate it > before activating to depth 2 (which loads the elements). > > blockSetFinished() : purpose of the deactivate()? > > difference between ArchivePutHandler and ContainerPutHandler should be > documented. some comments added in a34fd1794005b71cf7c04c16973aa80fe9f63653
> ContainerBuilder constructor : do the EMPTY_CHK_URI need cloning or are > they cloned in ArchivePutHandler/ContainerPutHandler constructor anyway? > *** ContainerBuilder is in the first place an 'internal' thing, so the caller is responsible for clone if neccessary. ArchivePutHandler/ContainerPutHandler clone them. comment added in b40a7990ca8953ed87c4cd44f81163fb65aa6c75 > > 36694fd123278b00235d1769fd085763fdba0a8e > > getCompressorsArray : trim codec
