Again, my concern is with documentation and direction, not necessarily going through and making these changes at right now.
On Wed, Apr 25, 2018 at 3:45 PM, Jinmei Liao <jil...@pivotal.io> wrote: > Any config object that needs to be Identifiable and Serializable should be > identified as a CacheElement, but we don't have to go through all of the > objects and identify them at once. When we are refactoring each individual > commands and recognize the need, we will identify the object to be a > CacheElement. > > On Wed, Apr 25, 2018 at 3:41 PM, Patrick Rhomberg <prhomb...@pivotal.io> > wrote: > > > Sorry, I forgot to step away from the In The Weeds where I am. > > > > o.a.g.cache.configuration.CacheElement is the experimental interface > meant > > to identify an extension's configuration classes for consumption with the > > new cluster configuration interface. Extension developers can then use > the > > configuration service to disseminate these configuration objects via > member > > groups, consistent with other configuration elements (e.g., indices and > > gateways). See [1] for the API's proposal. > > > > We also have been using this interface internally on some of our own > > configuration objects, for instance the JDBC's ConnectorService. This > > allowed us to use some utilities we had made for the easy retrieval of > the > > configuration objects. > > > > The lack of documentation on this interface is my core concern. The > > initial intent was to label those extension's configuration classes, but > > beyond that, its use is ambiguous. Even within that purview, it's not > > clear whether an inner class should be required to implement > CacheElement. > > > > Jinmei and I had a thread recently on a pull request of hers [2] > suggesting > > perhaps "anything you want to be searchable in the config" should have > the > > interface applied to it. That remains more subjective than I'd > personally > > like, but it would at least be documentable. > > > > So to return to my initial concerns: our current use of the interface is > > inconsistent, and with it being an external API, we should properly > define > > the interface's scope for both external and internal use. I propose four > > possible scopes in the initial message. > > > > @Udo, I haven't added a section on the linked proposal, but the page > could > > do with a larger update in any case. I'll start that straight away. > > > > [1] Public API for Cluster Configuration Proposal: > > https://cwiki.apache.org/confluence/pages/viewpage. > action?pageId=75975896 > > [2] PR #1853: https://github.com/apache/geode/pull/1853 > > > > On Wed, Apr 25, 2018 at 12:56 PM, Galen O'Sullivan < > gosulli...@pivotal.io> > > wrote: > > > > > It looks like there are two classes called CacheElement in the > codebase; > > > am I correct in thinking that you're referring to > > > org.apache.geode.cache.configuration.CacheElement? > > > > > > This class doesn't have a Javadoc, so it's a little hard as an outsider > > to > > > understand exactly what it is or how it's used. Can you explain the > > basics > > > of how it's used? > > > > > > Thanks, > > > > > > Galen > > > > > > > > > > > > On 4/19/18 5:03 PM, Patrick Rhomberg wrote: > > > > > >> Hello all! > > >> > > >> We introduced the CacheElement interface as part of our > experimental > > >> API > > >> to update the cluster configuration. I'd like to solidify and > document > > >> our > > >> intent for the interface and the extent to which it is expected to > > apply. > > >> In its current form, the CacheElement interface extends > Serlializable > > >> and > > >> Identifiable<String>. Serialization is required for communication > > between > > >> members, and Identifiable is useful during lookup, modification, and > > >> removal of an existing configuration object. > > >> At our current iteration, it is not entirely clear which of our own > > >> configuration objects should or should not implement CacheElement. I > > >> think > > >> one of following interpretations may be best, but don't know which > would > > >> be > > >> most natural. And, of course, if you have another that I've > overlooked, > > >> I'd love to hear it. > > >> > > >> Classes that implement CacheElement are... > > >> > > >> A) ... only custom configuration elements provided by extension > > >> developers. This declaration will appear at a top-level class > > declaration > > >> that is directly consumed by the CacheConfig or RegionConfig. > > >> > > >> B) ... custom configuration elements, as well as any configuration > > object > > >> that we wish to be searchable within the configuration. > > >> > > >> C) ... custom configuration elements, as well as any configuration > > object > > >> appearing at the same level in the configuration hierarchy. That is, > > >> every > > >> getter of both CacheConfig and RegionConfig should return either a > > >> CacheElement, or a List<CacheElement>. > > >> > > >> D) ... any object, excepting those in java.lang.*, that appears > anywhere > > >> in > > >> the cluster configuration's hierarchy, including all inner classes. > For > > >> example, ConnectorService.RegionMapping implements CacheElement. > > >> > > >> > > >> Each comes with its own potential pitfalls. > > >> Option (A) seems restrictive and sparse. > > >> Option (B) is subjective. > > >> Many classes do not have a `name' or `id' field, making option (D) > > >> difficult. > > >> I like option (C), although we have already moved beyond that > > >> specification in our current iteration, with > > >> ConnectorService.RegionMapping > > >> implementing CacheElement. > > >> > > >> Thoughts? > > >> > > >> Imagination is Change. > > >> ~Patrick Rhomberg > > >> > > >> > > > > > > > > > -- > Cheers > > Jinmei >
