Hi Chris,
this makes sense to me :-) If we do go down this path, we should
consider that some information is:
- driver specific: what capabilities does this particular protocol
implementation support
- protocol specific: what capabilities (for example
writing/subscription) does the protocol provide in general
- connection specific: for example, whether the connection is
encrypted, authentication/authorization used etc.
- device specific: what capabilities does the connected device provide
(might be a subset of protocol capabilities)
We should be careful when designing that metadata interface and not mix
these things up, to avoid confusion. For example, it should be clear to
the client that in case subscription is not supported, whether this is a
driver (protocol implementation) issue, a protocol issue, or device issue.
Andrey
On 10/08/2018 11:01 AM, Christofer Dutz wrote:
Hi Andrey,
Ah ok ... now I understand. I agree that I also like this approach ... it keeps
the connection cleaner.
And I guess such a Metadata object could not only contain such information
about the capabilities, but also the concrete type of the PLC a connection is
connected to, Versions etc.
I could imagine that some supported functions are not only limited by the
driver itself, but by the PLC model used. At least the supported datatypes is
highly dependent on the type of S7 device.
So I would definitely +1 to go down this Metadata path.
Chris
Am 07.10.18, 19:46 schrieb "Andrey Skorikov" <[email protected]>:
Hi Chris,
I agree. As for now, the PR is already quite large and I would not like
to let it grow further.
A metadata object returned by some operation on PlcConnection (for
example getMetadata() or getCapabilities()) would bundle all the
operations for obtaining information about the connection itself. This
is in contrast to the operational interface of the connection, which is
used to actually perform the operations like reading/writing. Basically,
all the canXYZ operations discussed so far can be bundled into one
interface, and that would constitute the management interface (for
obtaining metainformation) of the collection.
As Julian pointed out, this pattern is employed in the java.sql API:
https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html.
The corresponding operation to obtain an instance of that type is
https://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#getMetaData().
Another example is the JMX instrumentation level API for dynamic beans:
https://docs.oracle.com/javase/7/docs/api/javax/management/DynamicMBean.html#getMBeanInfo().
However, I believe that at this stage there is no need to provide a
separate interface for that, and having simple canRead()/canWrite()
directly on PlcConnection would be sufficient.
Andrey
On 10/07/2018 06:17 PM, Christofer Dutz wrote:
> Hi Julian,
>
> I agree that we should merge things asap ... just because something is
merged, doesn't mean we can't fine-tune it after that.
> I did have a look at the changes and I think it's safe to continue down
that path.
>
> Also I like the idea of getting rid of the Optional ... it was annoying me too for quite
some time. So having a "canXYZ" and a companion "getXYZRequestBuilder" methods
sounds perfect from my side.
> This way we can go the extra step of ensuring support, but can omit it
where we just don't need it.
>
> Haven't quite understood the whole "Metadata" thing though ... ;-)
>
> Chris
>
>
> Am 07.10.18, 15:15 schrieb "Julian Feinauer"
<[email protected]>:
>
> Hey all,
>
> one more question.
> Do we do the suggested changes in Andreys PR Branch or do we do it
separately.
> Then, we should try to merge this branch ASAP to have it there and
to avoid merge hell (see https://media.giphy.com/media/cFkiFMDg3iFoI/giphy.gif).
>
> Personally, I feel unable to do a Code Review in the original sense
(105 changes).
> So after going through the API changes I definitely +1 them but I'm unsure if
a "proper" Code Review is possible / necessary (so would agree on merging
directly).
>
> What do others think?
>
> Julian
>
> Am 06.10.18, 21:20 schrieb "Julian Feinauer"
<[email protected]>:
>
> Hey Andrey,
>
> I have to admit that your naming is definetly better than mine.
> And I like your idea about this Metadata thing a lot.
> I just checked how this is named in JDBC and the respective
class is https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html
>
> So I think we can provide a canRead / canWrite (canSubscribe is
a bit difficult, as we already hat several discussions about if we implement that
by polling by default).
> But I would also like the idea of having such a Metadata
interface to transport further information about the PLC (like if this is native
subscribing or polling and all such stuff).
>
> Best
> Julian
>
> Am 06.10.18, 21:08 schrieb "Andrey Skorikov"
<[email protected]>:
>
> Hello Julian,
>
> I think that a canRead()/canWrite()/canSubscribe() methods
signaling
> whether the connection supports
reading/writing/subscription is a really
> good solution. This would cleanly separate querying the
meta-information
> of a connection (whether the connection provides the
required
> capability) from actually using it, and would free the
client from
> dealing with the Optional<?>s all the time.
>
> There are also some alternative solutions:
>
> - Provide the meta-information in a separate data
structure, returned by
> some operation like getCapabilites() on PlcConnection. This
can be
> modeled in great detail or very simply (for example by
returning a
> BitSet). The client would check whether the required
operation is
> supported by calling operations on that object.
>
> - Provide "mix-in" interfaces, for example Readable and
Writable. The
> client would check whether the connection supports reading
by evaluating
> whether the connection object implements the required
interface (for
> example: connection instanceof Readable) and casting the
connection to
> that type.
>
> - Provide no meta-information at all and just throw an
exception when a
> unsupported operation is invoked. Would not recommend that,
but still :-)
>
> In total, I think that Julian's solution (canRead() with
Exception
> thrown when a unsupported operation is invoked) balances
the complexity
> and flexibility best.
>
> Andrey
>
>
> On 10/06/2018 08:38 PM, Julian Feinauer wrote:
> > Hey everybody,
> >
> > I’m currently groing through Andreys PR
(https://github.com/apache/incubator-plc4x/pull/27) which introduces some very good
API changes and makes the API a lot more concise.
> > But one thing that annoys me from the first day on plc4x is
still there (and is now even more annoying as the rest is so clean). It is the boilerplate
code I have write all the time when “just doing a connection to read something” due to the
Optional<?> for getting the reader (or now the ReadRequestBuilder).
> > For me, the getReader (or now readRequestBuilder) as
Optional is like what Sebastian hates about Checked Exceptions.
> > I never had to deal with a Connection which did not have
a Reader but I had to check the Optional… at least 50 times, perhaps even more.
> >
> > Can’t we come up with a solution for that which would
make the API (from my perspective) even more clean and user friendly.
> >
> > Suggestions could be:
> >
> > 1. Replace the connection directly with Reader, so no
getConnection but getReader (or readRequestBuilder). And if this fails, it throws a
PlcConnectionException, as usual.
> > 2. No optional but another or canRead() method (for
those who like it save) and it then throws a unchecked PlcConnectionException (or
some subclass)
> >
> > What do the others think? Is this only me having the
feeling that this is the same anti-pattern as with the checked exceptions?
> >
> > Julian
>
>
>
>
>
>
>
