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

Read Response with timestamped SenML #1610

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mgdlkundera
Copy link
Contributor

@mgdlkundera mgdlkundera commented Apr 19, 2024

Implementation of read response with timestamped SenML

This aims to implement : #1553

@mgdlkundera mgdlkundera changed the title Read Response Read Response with timestamped SenML Apr 19, 2024
@sbernard31
Copy link
Contributor

I will try to look at this tomorrow.

Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

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

My first quick review.

I didn't checkout / test the code yet.

Do you want to handle timestamped value for ObserveResponse too ?
(I mean for observe response, not notification (notification as already timestamp value)

@Override
public ReadResponse call() throws Exception {
// send a request with 3 seconds timeout
return server.send(registration, new ReadRequest(ContentFormat.SENML_JSON, 1), 2000);
Copy link
Contributor

Choose a reason for hiding this comment

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

2 seconds timeout is maybe too high ? 1 is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should leave a 2 seconds timeout, with 1 s we can't get a response

Copy link
Contributor

Choose a reason for hiding this comment

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

1s should be largely enough to get a response using localhost in tests.

So I read the test again and I see that the comment says :

  • 3 seconds
  • and the timeout value parameter value is 1

But the javadoc says :

 * @param timeoutInMs The global timeout to wait in milliseconds

So it should be 3 seconds and 3000 or 1 seconds and 1000

client.sendResponse(Type.ACK, ResponseCode.CONTENT, ContentFormat.SENML_JSON).loadMID("MID").loadToken("TKN")
.payload(payload).go();

ReadResponse response = future.get(2000, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

2 seconds timeout is maybe too high ? 1 is enough

(why not just using 1 and TimeUnit.SECONDS)

@sbernard31
Copy link
Contributor

I rebase the code on master and push some fixes.

You could review it if wanted. (I advice to review commit by commit)

We still need to decide :

Do you want to handle timestamped value for ObserveResponse too ?
(I mean for observe response, not notification (notification as already timestamp value)

I didn't have time to test about reducing timeout value but I pretty sure that 3 seconds is not needed

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.

None yet

2 participants