Bug 333018

Comment 1 Dean Roberts 2011-01-04 10:02:50 EST

I propose adding API to SimpleProfileRegistry.

The implementation will be a single Java Property file for each profile directory.  The file will contain all the properties for all the states (timestamps) for the profile directory.  The property keys will be manufactured as timestamp.userkey.

The set operations will do the following:
1) Lock the property file
2) Read the file
3) Add properties to in memory model
4) Write the file
5) Unlock the file
6) Null the in memory model (*)

The get operation will do the following:
1) Lock the property file
2) Read the property file
3) Unlock the property file
4) Return the appropriate values
5) Null the memory model (*)

I would like opinions on whether we should really be discarding the memory model after each operation or if we should keep the memory model which would allow us to check the file timestamp before re-reading the file if it has not changed.  

If we keep the memory model around, do we keep it around forever or do we need a strategy for discarding it after some amount of inactivity?

I propose the following API additions to SimpleProfileRegistry.

	/**
	 * Set properties on a profile state.  Overwrite existing properties if present.
	 * 
	 * @param profileID
	 * @param timestamp
	 * @param properties
	 * @throws NullPointerException if either profileID or properties are null 
	 * @throws IllegalArgumentException if the state timestamp does not exist for the profile
	 */
	public void setProfileStateProperties(String profileID, long timestamp, Map<String, String> properties) {
		// TODO: Implement
	}

	/**
	 * Set a specific property on a profile state.  Overwrite existing properties if present.
	 * Setting a property value to null removes the property from the profile state
	 * 
	 * @param profileID
	 * @param timestamp
	 * @param key
	 * @throws NullPointerException if either profileID or key is null
	 * @throws IllegalArgumentException if the state timestamp does not exist for the profile
	 */
	public void setProfileStateProperty(String profileID, long timestamp, String key, String value) {
		// TODO: Implement
	}

	/**
	 * Return all properties for a profile state.
	 * @param profileID
	 * @param timestamp
	 * @return a property map of key value pairs.  An empty map if the profile state has no properties or does not exist
	 * @throws NullPointerException if profileID is null.
	 */
	public Map<String, String> getProfileStateProperties(String profileID, long timestamp) {
		// TODO: Implement
		return null;
	}

	/**
	 * Return a map for each profile state that has a key defined with a timestamp/value pair for each state
	 * in the profile that defines the property key.  The value returned is the property value for key for
	 * the timestamp.
	 * 
	 * @param profileID
	 * @param timestamp
	 * @param key
	 * @return A map of timestamp and values for the given key.  An empty map if no states define the given key.
	 * @throws NullPointerException if  profileID is null.
	 */
	public Map<Long, String> getProfileStateProperties(String profileID, long timestamp, String key) {
		// TODO: Implement
		return null;
	}

	/**
	 * Remove all properties with matching keys from the given profile state.  Non-existent keys are
	 * ignored.  If the state does not exist the method performs a no-op and returns normally.
	 * @param profileID
	 * @param timestamp
	 * @param keys
	 * @throws NullPointerException if profileID, keys or any element of keys is null.
	 */
	public void removeProfileStateProperties(String profileID, long timestamp, String[] keys) {
		// TODO: Implement
	}

	/**
	 * Remove all properties for the given profile state.  If the state does not exist the method performs a no-op and returns normally.
	 * @param profileID
	 * @param timestamp
	 * @throws NullPointerException if the profileID is null.
	 */
	public void removeProfileStateProperties(String profileID, long timestamp) {
		// TODO: Implement
	}

Comment 2 Dean Roberts 2011-01-04 11:14:51 EST

Sorry there was a typo in the method signature of one of the API methods.  The method that takes a profileID and a key and returns a Map<long, String> of all profile states that define the key should read as follows

    /**
     * Return a map for each profile state that has a key defined with a
timestamp/value pair for each state
     * in the profile that defines the property key.  The value returned is the
property value for key for
     * the timestamp.
     * 
     * @param profileID
     * @param key
     * @return A map of timestamp and values for the given key.  An empty map
if no states define the given key.
     * @throws NullPointerException if either profileID or key is null.
     */
    public Map<Long, String> getProfileStateProperties(String profileID, String key) {
        // TODO: Implement
        return null;
    }

Comment 3 Dean Roberts 2011-01-04 11:24:37 EST

Additionally, I think the return type of the method should just be Map<String, String> instead of Map<Long, String>, where the key is the String representation of the long timestamp.

When constructing the return values that implementation has the timestamp as a String already.  The caller can convert the String to a long if they really need the long ... but in all likely hood they probably need the String

Comment 4 John Arthorne 2011-01-04 15:14:10 EST

(In reply to comment #1)
> I propose adding API to SimpleProfileRegistry.

SimpleProfileRegistry is not an API class. I assume you mean adding API on IProfileRegistry with default implementation in SimpleProfileRegistry.

> I would like opinions on whether we should really be discarding the memory
> model after each operation or if we should keep the memory model which would
> allow us to check the file timestamp before re-reading the file if it has not
> changed.  

I think it will need to keep it around. The common use case will be:

 - Client calls listProfileTimestamps
 - client calls getStateProperty on each state to discover which ones it cares about

If it went to disk for each profile state it would not scale well, and defeats the purpose of using a single file for all states of a given profile id.

 
> If we keep the memory model around, do we keep it around forever or do we need
> a strategy for discarding it after some amount of inactivity?

You could try just keeping the state properties for the most recently accessed profile (a cache of size 1). That would limit the memory consumption, and could be converted to a SoftReference if needed.

Comment 5 Dean Roberts 2011-01-05 11:23:21 EST

Created attachment 186096 [details]
Add profile state meta data API to IProfileRegistry

Added an initial implementation of the proposed API with some automated tests.  I'll now look at using the API to support hiding of certain profile states in the installed updates UI.  Pascal was also interested in looking that the API in regards to tagging certain profile states with descriptive names.

This implementation does not cache the meta data between reads.  It seemed from our meeting before Christmas that people did not want this and we tried to pick an API that could return all relevant information in a single call.

The validity of this assertion should become apparent as we try and use the API to do useful work.

Comment 6 DJ Houghton 2011-01-13 16:42:44 EST

Created attachment 186784 [details]
patch

Updated patch with a few changes:
- completed javadoc
- added caching for the last accessed profile state properties object
- the "remove" tests were failing seemingly randomly, turns out there was a line which removed a property from a "state timestamp + 1" which sometimes would do work and other times, not. Either way, it didn't fail. (is spec'd to no-op if state doesn't exist)
- random formatting/code style changes (i.e. buffer output stream, helper methods for commonly performed actions, commented complicated code, etc.)

John, I think the only thing left to review is the names of the new API methods. They seem wordy to me but I'm not sure if we want to shorten them as it might lead to confusion with the properties right on profiles themselves. Thoughts?

Also I am not familiar with profile locking but we seem to use it a lot here in this new code, but not anywhere else. Should we get rid of all the lock/unlock calls or keep them?

Comment 7 Pascal Rapicault 2011-01-16 21:38:41 EST

I have reviewed the API and the code. Overall I like it. I have the following comments:
On the API signature, I'm not a fan of throwing an IllegalAccessException when the state does not exist. I'd rather return a boolean on success or failure.
On the code side, I would be tempted to implement setProfileStateProperty(String id, long timestamp, String key, String value) using the other setProfileStateProperty method.

Comment 8 DJ Houghton 2011-01-18 10:37:30 EST

The only problem with returning a boolean in the case of failure is with reporting a detailed error message. We would lose context. The other option would be to throw a checked exception.

Comment 9 Jeff McAffer 2011-01-18 11:12:21 EST

or a status

Comment 10 DJ Houghton 2011-01-18 11:13:42 EST

ha! brain fart, forgot about those status objects. I'll do that.

Comment 11 DJ Houghton 2011-01-18 17:14:45 EST

Created attachment 187055 [details]
patch

Comment 12 John Arthorne 2011-01-19 09:28:42 EST

I reviewed the patch with DJ and made a couple of minor suggestions.

Comment 13 DJ Houghton 2011-01-19 10:37:41 EST

Created attachment 187117 [details]
patch

Updated patch.

Comment 14 DJ Houghton 2011-01-19 10:38:31 EST

Patch released.