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

Task/3068 ngsiv2 update forwarding #3480

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

Conversation

kzangeli
Copy link
Member

Implementation of NGSIv2 forwarding of updates for the request

PATCH /v2/entities/{EntityID}/attrs

@@ -1,4 +1,5 @@
- Fix: idPattern '.*' is now allowed in NGSIv2 registrations (#3458)
- Add: Forwarded queries work for registrations using idPattern == '.*', but only for simple cases (#3458)
- Add: Forwarded updates work for registrations using idPattern == '.*', but only for simple cases (#3458)
Copy link
Member

Choose a reason for hiding this comment

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

This line seems to be duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

One line is about queries, the other about updates.
They are very similar but also completely different

Copy link
Member

Choose a reason for hiding this comment

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

I misses the "queries" vs "udpates" difference. I's ok. Sorry the noise.

NTC

@@ -16,7 +16,7 @@ In NGSIv1 (deprecated), the request `POST /v1/updateContext` has a field called
* APPEND_STRICT
* REPLACE

> Side-node: The first three are "standard NGSIv1" while the second two were added for NGSIv2.
Copy link
Member

Choose a reason for hiding this comment

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

This file modificaiton seems to be related with fixes in the query forward documentation, but the update forwared documetnation about (FW-02 if I'm remembering correctly) has not be modified. Probably an update of the png for FW-02 is needed also, but I haven't seen yet the changes in the code so I'm not fully sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you probably can see, the modifications in this file are just corrections.

About documenting Update Forward, part of it is added, but most probably not all that is necessary

forwarded updates do not work for registrations with idPattern (the plan is to fix this asap).
And, right now (2019-04-24), forwarding for registrations with idPatterns works for queries using `GET /v2/entities`
as well as updates using `PATCH /v2/entities/{Entity-ID}/attrs`.
More requests will be added to this list.
Copy link
Member

Choose a reason for hiding this comment

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

I think is better the old wording, without providing the detail of the particular operations. At least until the translation of cases/0787 test so we can have more insight in which operations are supported and which operations doesn't.

Copy link
Member Author

Choose a reason for hiding this comment

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

Until we have a clear path forward, let's wait with these kind of changes. No need to spend time before we are sure what we want exactly

Copy link
Member

Choose a reason for hiding this comment

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

... and if there is no need of spending time, why this file was modified? ;)

I'd suggest to go back for its previous version.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following a perhaps unclear spec and this was my interpretation, a logical one by the way.
I did not see this as spending time for nothing.

@@ -2703,17 +2703,24 @@ static void searchContextProviders
std::string err;
Copy link
Member

@fgalan fgalan Apr 29, 2019

Choose a reason for hiding this comment

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

Is there any change in this file appart from LM_T LmtForward adddition? It seems so but I would like to have a confirmation from your side, pls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to be only LM_T in this file, yes

Copy link
Member

Choose a reason for hiding this comment

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

NTC

TIMED_RENDER(payload = eP->toJson(NGSI_V2_NORMALIZED, nullFilter, false, nullFilter));

resource = prefix + "/entities/" + eP->id + "/attrs";
verb = "PATCH";
Copy link
Member

@fgalan fgalan Apr 29, 2019

Choose a reason for hiding this comment

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

I'm afraid this is contrary to specification #3068 (comment):

  • POST /v2/op/update will use "update" as actionType (to be consistent with NGSIv1)

That's the operation which has to be used to implemente NGSIv2 forwarding.

Copy link
Member Author

Choose a reason for hiding this comment

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

No really contrary to the "specification" though.
You are talking about batch ops (POST /v2/op/update). This PR only deals with the PATCH operation.

As before, once we have agreed on a way to attack the forwarding, this will either stay the same or be modified.
Before we have that agreement, nothing can be done really.

@@ -25,6 +25,9 @@
#include <string>
#include <vector>

#include "logMsg/logMsg.h"
Copy link
Member

Choose a reason for hiding this comment

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

Really needed in this file? I don't see any change in this file using the things added by these includes...

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably had an LM_T and then removed it.
These things happen during PRs.
The inclusion of logMsg.h doesn't hurt (except for a millisecond extra during compilation)

Copy link
Member

@fgalan fgalan Apr 29, 2019

Choose a reason for hiding this comment

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

I agree it doesn't hurt, but let's try to keep files tidy. If an include isn't used by a file, then it should be removed (if I'm remembering correctly, I think we have some rules about that in style guide).

@@ -715,6 +715,14 @@ function accumulatorStart()
shift
fi

url="/notify"
Copy link
Member

@fgalan fgalan Apr 29, 2019

Choose a reason for hiding this comment

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

This is a fix in harness suite? I mean, until this fix the --url parameter was being ignored?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it seems so ...
I needed to change the accumulator to accumulate the request for update forwards and as this was already implemented I decided to use it. Didn't really work though, not without this modification

Copy link
Member

Choose a reason for hiding this comment

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

NTC

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