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

support dataflow affinity using any preceding data operations #4324

Merged
merged 5 commits into from
Oct 8, 2024

Conversation

xliuqq
Copy link
Collaborator

@xliuqq xliuqq commented Sep 21, 2024

Ⅰ. Describe what this PR does

support dataflow affinity using any preceding data operations, more details see fluid-cloudnative/community#68

Ⅱ. Does this pull request fix one issue?

fixes #XXXX

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

unit test.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Copy link

fluid-e2e-bot bot commented Sep 21, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: xliuqq <[email protected]>
@xliuqq xliuqq marked this pull request as ready for review September 22, 2024 02:08
Copy link

sonarcloud bot commented Sep 25, 2024

Comment on lines +53 to +57
dependOnOp := runAfter.AffinityStrategy.DependOn
if dependOnOp == nil {
dependOnOp = &runAfter.ObjectRef
}

Copy link
Member

Choose a reason for hiding this comment

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

Current logic allows users to dependOn a data operation which is not in the dataflow. For example:

apiVersion: data.fluid.io/v1alpha1
kind: DataProcess
metadata:
  name: step4-infer-server
spec:
  runAfter:
    kind: DataLoad
    name: step3-warmup-cache
    namespace: default
    affinityStrategy:
      # get affinity from which data operation
      dependOn:
        kind: DataProcess
        name: some-other-op
        namespace: default
      policy: Require
        # Require to run on a node with the same label value as the dependent operation
        requires: 
        - name: node.kubernetes.io/instance-type

Is it by design? If not, is it possible to check if the spec.affinityStrategy.dependOn object is a preceding data operation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the context of location affinity, I think there is indeed an implicit temporal dependency. By specifying affinity, we need ensure that a particular data operation runs on specific nodes where previous operations might have already done.

Copy link
Collaborator

@cheyang cheyang left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link

fluid-e2e-bot bot commented Oct 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheyang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cheyang cheyang merged commit 9f269b4 into fluid-cloudnative:master Oct 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants