Skip to content

converter implementation#499

Closed
ankitk-me wants to merge 23 commits into
aklivity:developfrom
ankitk-me:convertor
Closed

converter implementation#499
ankitk-me wants to merge 23 commits into
aklivity:developfrom
ankitk-me:convertor

Conversation

@ankitk-me

Copy link
Copy Markdown
Contributor

No description provided.

[
"json"
]
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to put validator implementation-specific properties in the validator-specific schema patches so that engine doesn't need global knowledge of the possible properties used by all the validator implementations.

This currently includes

  • encoding
  • expect

Please refactor them out of the engine schema.

int length)
{
return validate(data, index, length);
return length == 18 ? data : null;

@jfallows jfallows Oct 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not quite right because it loses information about index and length.

Take a look at MessageConsumer, MessageFunction and MessagePredicate in io.aklivity.zilla.runtime.engine.binding.function package.

I think we might need the following in io.aklivity.zilla.runtime.engine.validator.function package:

  • ToIntFragmentFunction
ToIntFragmentFunction
  int applyAsInt(int flags, DirectBuffer buffer, int index, int length)
Validator
  int validate(int flags, DirectBuffer buffer, int index, int length, ToIntFragmentFunction next)

The return value from applyAsInt is the progress towards length, and the implementation would write the validated (converted) value to next.

public class TestValidator implements Validator
{
    public int validate(
        int flags,
        DirectBuffer data,
        int index,
        int length,
        ToIntFragmentFunction next)
    {
        return length != 18 ? -1 : next.applyAsInt(flags, buffer, index, length);
    }
}

So the lambda we pass in for next would actually write the value into the next step in the pipeline.

A better implementation would track the state of progress towards 18 bytes in this case, but that makes it stateful, which is another level of complexity we need to sort out after getting the interfaces to support streaming.

What do you think?

@jfallows jfallows Oct 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alternatively, we can consider using MessageConsumer more directly in validator, if that makes it simpler to propagate valid DATA frames.

public class TestValidator implements Validator
{
    public int validate(
        int msgTypeId, // DataFW.TYPE_ID
        DirectBuffer buffer,  // contains entire DataFW frame
        int index,
        int length,
        MessageConsumer next)
    {
        return length != 18 ? -1 : next.applyAsInt(msgTypeId, buffer, index, length);
    }
}

However, this may be to skewed towards DATA frames, as we also need to validate headers, etc.

}

return value;
return value.capacity() == 0 ? -1 : next.applyAsInt(value, index, value.capacity());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that we don't have to return the value, the allocations for value, payloadBytes, byteBuf, valBytes can be eliminated, or reuse shared instances as necessary that are only needed on stack during method invocation, so no chance of concurrent reuse.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note the return value here should be how much of length was processed, whereas the return value of next is an indication of how much of what was passed into next was processed.

"{\"name\":\"status\",\"type\":\"string\"}]," +
"\"name\":\"Event\",\"namespace\":\"io.aklivity.example\",\"type\":\"record\"}";

private final ToIntValueFunction fragmentFunction = (buffer, index, length) -> length;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be static too, right?
Perhaps called NEXT_NOP ?

ToIntValueFunction next)
{
return validate(data, index, length) ? data : null;
return !validate(data, index, length) ? -1 : next.applyAsInt(data, index, length);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is all good to return the progress from next because there is no transformation, so the progress in next is equivalent to the progress in this validator.

ankitk-me and others added 14 commits October 18, 2023 18:53
* feature/m1-docker-build-support

- separate jammy and alpine
- add zilla version as env var
- add the docker platform to properties
- don't need to use alpine for build

* docker image tagging options

separate alpine base image from the default image and add more tagging options

* set the version env var in the alpine build

* remove the suffix for local build

* make version tagging more explicit for each profile

* move the alpine specific builds into the docker-image module

* reduce the folder complexity and add child pom placeholders

* revert the docker-image pom to develop

* Use buildx for multi-arch images, build alpine image for release only

* Move inline assembly to descriptor file and reference from alpine image

---------

Co-authored-by: John Fallows <john.r.fallows@gmail.com>
)

Bumps alpine from 3.18.3 to 3.18.4.

---
updated-dependencies:
- dependency-name: alpine
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ankitk-me ankitk-me closed this Oct 30, 2023
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.

6 participants