Hi Matt, I have directly replied to your comments on the jira.
Thanks Amit On Sat, Aug 27, 2016 at 4:22 AM, Matt Ryan <o...@mvryan.org> wrote: > Use this patch instead; updated patch from latest in trunk. > > On Fri, Aug 26, 2016 at 4:41 PM, Matt Ryan <o...@mvryan.org> wrote: > >> Hi Oak Devs, >> >> I've created OAK-4712 and submitted a patch for the same. I've attached >> the same patch to this email. >> >> The submission is to add a new MBean, S3DataStoreStats, which will allow >> reporting via JMX about the state of the S3DataStore. Two metrics are >> intended. The first is to report the number of files that are in the sync >> state, meaning they have been added to the S3DataStore but are not yet >> completely copied into S3. The second is to allow to query the sync state >> of a file - a return value of true would mean that the file provided is >> fully synchronized. This uses an external file name, not the internal ID. >> >> I have some questions about the implementation first: >> 1. For the request to query the sync state of a file, should the file >> name provided be a full path to a local file or a path to a file within OAK >> (e.g. /content/dam/myimage.jpg)? Current implementation uses a local file >> path but I have been wondering if it should be an OAK path. >> 2. For the request to query the sync state of a file, when converting >> from the externally-supplied file name to an internal DataIdentifier, this >> implementation is performing the same calculation to determine the internal >> ID name as is done when a file is stored. I have a number of concerns with >> this: >> - It is inefficient - the entire file has to be read and digested in >> order to compute the internal ID. This takes a long time for large assets. >> - I've essentially duplicated the logic from CachingDataStore into >> S3DataStore to compute the internal ID. I hate duplicating the code, but I >> am trying to avoid exposing internal IDs in API, and I am not seeing a good >> way in the current implementation to avoid this without either modifying >> public API to CachingDataStore, or exposing the internal ID via API, or >> both. >> >> Any suggestions on these two issues? >> >> >> I'm also experiencing a problem with this patch. In my testing it >> appears to work fine, until I delete a file. For example, if I delete an >> asset via the REST API, I will see the asset deleted in CRXDE. However, >> the file still remains in S3. This MBean as implemented only knows how to >> check with S3DataStore and the corresponding backend, and these all appear >> to believe the file still exists. So the MBean continues to report that >> the file's sync state is synchronized (i.e. isFileSynced() returns true) >> even though the file has been removed from the JCR. Any suggestions on how >> to resolve this? >> >> >> Finally, any feedback on the patch is welcome. And if I did the process >> wrong please correct me (gently) - first time submission here. Thanks >> >> >
