I understand your patch is great for consistency (no chance the two will
get out of sync now) and I agree that it is hard to write a test case
before (it involves the type of the file changing on disk after the
datastore has already been created).
Can you also patch the ReType code to respect the result of
FeatureType.equals(FeatureType)? That is it should be able to "re type"
even if all that changes is which geometry is marked as the default.
--
Jody Garnett
On 7 January 2015 at 14:51, Torben Barsballe <[email protected]>
wrote:
> Here is a pull request fixing the retyping problem:
> https://github.com/geotools/geotools/pull/668
> Because the issue only crops up sporadically, I am not sure I can create a
> usefull test case...
>
> Torben
>
> On Wed, Jan 7, 2015 at 11:12 AM, Torben Barsballe <
> [email protected]> wrote:
>
>> I appear to have found a deeper root cause of this problem and it looks
>> like it does not actually have much to do with locking.
>> My origional thoughts about the cause of the error appear to be in error
>> - this particular instance is correct in failing here as that is what the
>> test in question is intended to test. My confusion comes from the fact that
>> this test would run, through an (expected) exception, and then all
>> subsequent test methods in the same test file would fail with a
>> ServiceException, reminicient of a locking failure.
>>
>> Note that there are still some issues with locking in addition to this
>> problem.
>>
>>
>> It turns out that this failure was in part caused by an issue with
>> retyping:
>>
>> The test file used for the locking tests contains two geometry attributes
>>
>> When ContentDataStore creates a retyping reader, it creates a target
>> featureType using
>> 605: SimpleFeatureTypeBuilder.retype(getSchema(),
>> query.getPropertyNames());
>>
>> It then compares this target featureType to the featureType of the reader
>> to test if a retyping reader is actually neccessary:
>> 609 if ( !target.equals( reader.getFeatureType() ) ) {
>> 610 reader = new ReTypeFeatureReader( reader, target, false );
>> 611 }
>>
>> The error is introduced by our two geometries; ultimately getSchema()
>> (sometimes) returns a featureType with a different defaultGeometry then
>> that used by the reader (I suspect this is caused by arbitrary collection
>> ordering of the attributes). This caused the .equals call to return false,
>> despite the fact that both featureTypes have the same attributes. Then,
>> when we go to build the ReTypeFeatureReader, we get a failure because both
>> schemas have the same attribures.
>>
>> This failure is returned to WFS as a ServiceException.
>>
>> The problem can be fixed by changing line 605 of ContentDataStore to
>> 605: SimpleFeatureTypeBuilder.retype(reader.getFeatureType(),
>> query.getPropertyNames());
>>
>> I am currently testing this change, and should have a pull request
>> shortly.
>>
>> One question I have is if there is any way to ensure that we always get
>> the same defaultGeometry when we create a schema?
>>
>> Torben
>>
>>
>> On Tue, Jan 6, 2015 at 10:44 AM, Torben Barsballe <
>> [email protected]> wrote:
>>
>>> Oops, it looks like ContentState tracks DiffTransactionState, so the
>>> DiffContentFeatureWriter can actually access the DiffTransactionState via
>>> the ContentState. While this invalidates some of my previous points, it
>>> still might be worthwile to consider storing the flag in ContentState,
>>> since much of the work we will are disabling is done through ContentState.
>>>
>>> Torben
>>>
>>> On Tue, Jan 6, 2015 at 10:37 AM, Torben Barsballe <
>>> [email protected]> wrote:
>>>
>>>> Following along here:
>>>>
>>>>> Adding an extra isClosing field to DiffTransactionState seems the
>>>>> cleanest approach.
>>>>>
>>>>
>>>> Using this method, we give the writer in DiffTransactionState the
>>>> transaction.
>>>> This transaction gives the lock writer the correct authorizations to
>>>> pass the checks
>>>> The isClosing field is then accessed by the diff writer. If set to
>>>> true, the diffwriter doesn't write anything
>>>>
>>>> Based on the implementation, it looks like it might be better to put
>>>> the isClosing field in ContentState:
>>>> - The diff writer (DiffContentFeatureWriter) can access the Diff, the
>>>> ContentState, and the DataStore, but not the DiffTransactionState
>>>> - DiffTransactionState can access the Diff and the ContentState, but
>>>> cannot directly access the diff writer.
>>>> - The ContentState is unique to the transaction, and stores state
>>>> info, so is at least somewhat appropriate for such a flag
>>>>
>>>> If this implementation is a bad idea for some reason, please tell me -
>>>> I am still not entirely familiar with how Diffs, Transactions, and Locking
>>>> behave and interact in geotools.
>>>> If there is a better place to put this flag where it can be set by
>>>> DiffTransactionState and still accessed by DiffContentFeatureWriter then I
>>>> am open to suggestions.
>>>>
>>>> Torben
>>>>
>>>
>>>
>>
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
