Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Naive implementation of UpgradeProtocol #38

Open
wants to merge 12 commits into
base: jtango-9-lts
Choose a base branch
from
Open

Conversation

Ingvord
Copy link

@Ingvord Ingvord commented Feb 21, 2020

More to think about:

  1. fire-and-forget ZMQ sockets (PUSH/PULL, PUB/SUB) for async messages
  2. Marshaller/Unmarshaller performance
  3. Server side performance:
    • transaction mode
    • blackbox
    • device lookup
  4. Client side thread pools
  5. Non-blocking IO

Copy link

@altavir altavir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the general idea. I think we need to discuss message format and payload format more. For now it still tied to CORBA serialization strategy.

// ==========================================================================

public double readAttributeDouble(String name) throws DevFailed {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lacking documentation

try {
TangoMessage<Double> message = new TangoMessage<>("read", this.get_name(), name, TangoConst.Tango_DEV_DOUBLE, 0D);

message = marshaller.unmarshal(transport.send(marshaller.marshal(message)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug log entry here?

@@ -2020,6 +2080,10 @@ private static void checkDuplication(String[] list, String orig) throws DevFaile
*/
//==========================================================================
protected void finalize() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finalize is deprecated in Java 9 and is discouraged even before. Should use AutoCloseable and implement close mrthod.

import java.util.stream.Collectors;

/**
* @author Igor Khokhriakov <[email protected]>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short documentation required


import com.google.common.collect.Collections2;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest remove Guava if possible. Using mix of Guava and Java 8 is confusing. Most Guava functions have analogues in Java 8.


@Override
public TangoMessage unmarshal(InputStream stream) {
return gson.fromJson(new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8)), TangoMessage.class);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential bug here. If there are subsequent reads from the same stream, BufferedReader will read its buffer size and the next reader will start in the wrong place. If the stream is exhausted after read, it should be closed.


@Override
public TangoMessage unmarshal(InputStream stream) {
BufferedReader br = new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above

public String device;
public String target;
public int dataType;
public T value;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No serialization strategy here. I really do not like using toString for something other than debug. There should be either additional decoder/encoder strategy or limits on T.

}

@Override
public void connect(String endpoint) throws IOException {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging here? Also there probably should be a check about connecting, when connection already exists.

@Ingvord
Copy link
Author

Ingvord commented Feb 22, 2020

@altavir thanks a lot for your input!

@Ingvord Ingvord mentioned this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants