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

[HELIX-524] Add a getProgress to the Task interface #6

Closed
wants to merge 2 commits into from

Conversation

hongbozeng
Copy link

[HELIX-524] Add a getProgress to the Task interface, this is very helpful for long running tasks, from which we know the status of a task and see if it's blocked. The return value is a double, ranging from 0 to 1.0, 1.0 indicates a task is finished

@kanakb
Copy link
Member

kanakb commented Oct 2, 2014

I like this in general, but I have a couple questions/comments:

  • This is an interface change. Is there a way we can provide this functionality without requiring that all existing task implementations be rewritten?
  • What is the persistence story (if any)? Who calls getProgress?

@brandtg
Copy link

brandtg commented Oct 2, 2014

For persistence story, what about putting a new health report: /STAND_ALONE/INSTANCES/{instanceName}/HEALTHREPORT/taskStatus?

Another thing to consider is that adding #getProgress to the task interface requires users to be able to intelligently report task status. I suspect that in practice someone might be annoyed at this extra responsibility, and provide dummy numbers (sounds stupid, but saw it before).

Maybe a better approach would be to try to monitor on things we know a priori about task (e.g. lifetime) and provide tools to inspect ones that seem stuck (e.g. task/partition-addressable stack trace)?

@brandtg
Copy link

brandtg commented Oct 2, 2014

Wait, that stuff's gone now? Ok never mind.

Is persistence of the state of a running task completely necessary? (Like would JMX suffice?)

@hongbozeng
Copy link
Author

  • About the interface change, IMHO, this is a nature part of the task interface. If the existing customers want to update, adding the support is encouraged. We can have another interface for monitoring the progress, but doesn't seem to be a nice design.
  • The task framework should call the progress interface and expose the status somehow, discussed below.
  • Persistency story, ZK is a good place if we only want to record the final result. If we want to expose the progress as a task runs, putting these periodical status updates in ZK is not a choice due to the large traffic, generally ZK is not a good place for reporting and monitoring service status. I also discussed this with Jason, we thought about inGraph (which is not an option for open source), Kafka or Riemann. (Greg, JMX sounds a good idea.) Without a conclusion of where to put these status stats, I agree that the progress interface is not of much value. For the first step, it would be good enough if we can monitor the progress. What do you guys think?
  • For the bogus progress number, it's the customer themselves who need to track the progress, if they want to see the bogus value, I'm fine with that :). The controller should be set not to act on the bogus value by the customers.

@kanakb
Copy link
Member

kanakb commented Oct 3, 2014

Why not have an abstract ProgressReportingTask that implements task and includes all the JMX persistence, and then have tasks extends from that and implement getProgress()? That avoids the interface change, but also allows "smarter" tasks to let their progress be known.

@hongbozeng
Copy link
Author

Sorry for the delay. Are there any customers outside the Espresso team? If we are the only customer right now, it would not be too costly to update the implementation with the interface. Making the getProgress into the interface has a good thing that it naturally goes into TaskRunner and we don't need to do something like "instance of ProgressReportingTask" and then call the getProgress, or a subclass of TaskRunner which is specific for ProgressReportingTask and call the getProgress.

@kanakb
Copy link
Member

kanakb commented Oct 9, 2014

Since this is an open source project, the real answer to this question is "I don't know." We've made two public releases with this change, and so if there is anyone who has integrated or is planning to integrate with the task framework, they would need to make a change.

If ProgressReportingTask handles the JMX reporting itself (it's an abstract class, not an interface), then TaskRunner wouldn't need to care about progress at all.

And back to the "bogus" return value discussion, I think customers who don't have a good sense of progress shouldn't be required to change their integration just to implement getProgress().

@kanakb
Copy link
Member

kanakb commented Oct 9, 2014

Email from Kishore:

Apologize to throw in another idea into the mix. Why not have additional methods in the implementation that the agent can invoke on demand based on annotation?. This extends the way we invoke methods on the statemodel.

For example we have

@Transition(TO=MASTER, FROM=SLAVE)
void fromSlaveTOMaster(Message m, NotificationContext ctx){

}

Can we have something similar as 

@Method(name="getProgress")
Response getProgress(Message m, NotificationContext ctx){

}

Helix provides a generic way to invoke a method on a partition. I think this is more powerful , does not disturb any interfaces and can be extending to do custom stuff.
Also these methods will not be invoked via ZK, instead the controller can directly invoke the method on the participant.

Feedback?

@kanakb
Copy link
Member

kanakb commented Oct 9, 2014

I think having a @ProgressReporter or @TaskMethod(name="getProgress") could be an elegant way to allow users to plug in additional functionality. Progress could be just the first thing a task could expose, and in the future if we think of others, we just need to add a new annotation.

@zzhang5
Copy link
Contributor

zzhang5 commented Oct 9, 2014

If we are not using ZK to invoke these methods, are we opening some kind of end-point e.g. via Netty or JMX on each participant?

@kanakb
Copy link
Member

kanakb commented Oct 10, 2014

Kishore from JIRA comments:

Yes, thats the idea. With helix-ipc this will be possible rt? This can be
extended to write rebalancers that talk to nodes to get the high water mark
to decide new master. what do you think?

Jason from JIRA comments:

Yes. Helix-IPC will do the job. We can also extend the idea to get high
water mark, etc. Helix task frame exposes Task interface, so users are not
implementing StateModel directly. How can we make this available to Task
also?

@hongbozeng hongbozeng closed this Apr 11, 2016
kaisun2000 pushed a commit to kaisun2000/helix that referenced this pull request Oct 2, 2020
# This is the 1st commit message:

09/28 fix race condition t1

# This is the commit message #2:

09/28 fix race condition t2

# This is the commit message #3:

09/28 fix race condition t3

# This is the commit message #4:

09/28 fix race condition t4

# This is the commit message #5:

09/28 fix race condition t5

# This is the commit message apache#6:

09/28 fix race condition t6
micahstubbs added a commit to micahstubbs/helix that referenced this pull request Jun 1, 2022
parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654121552 -0700

# This is a combination of 2 commits.
# This is the 1st commit message:

parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654121488 -0700

ng update @angular/cli --from=5.2.11 --to=6 --migrate-only

add engines, remove volta since it conflicts with necessary global angular-cli

ngx-dag@0.0.2 --> @swimlane/ngx-graph@6.0.0

npm i -D rxjs-tslint, add rxjs specific linting rules, add lint:tslint alternative linting script

npm install @angular/cdk@6.1.0

# This is the commit message apache#2:

npm install @angular/{animations,cdk,common,compiler,compiler-cli,core,forms,material,platform-browser,platform-browser-dynamic,platform-server,router}@6.1.0

# This is the commit message apache#5:

npm i ajv@6.9.1

# This is the commit message apache#6:

npm i codelyzer@6.0.1

# This is the commit message apache#7:

npm i ngx-clipboard@11.1.5

# This is the commit message apache#8:

npm i tsickle@0.32.1

# This is the commit message apache#9:

restore rxjs-compat@6.0.0-rc.0 to fix rxjs issue at npm run build

# This is the commit message apache#10:

WIP state with new Reactive Form

# This is the commit message apache#11:

Revert "WIP state with new Reactive Form"

This reverts commit f9e2e37.
micahstubbs added a commit to micahstubbs/helix that referenced this pull request Jun 1, 2022
parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654123923 -0700

# This is a combination of 2 commits.
# This is the 1st commit message:

parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654123883 -0700

ng update @angular/cli --from=5.2.11 --to=6 --migrate-only

npm i -D rxjs-tslint, add rxjs specific linting rules, add lint:tslint alternative linting script

npm install @angular/cdk@6.1.0

# This is the commit message apache#2:

npm install @angular/{animations,cdk,common,compiler,compiler-cli,core,forms,material,platform-browser,platform-browser-dynamic,platform-server,router}@6.1.0

# This is the commit message apache#5:

npm i ajv@6.9.1

# This is the commit message apache#6:

npm i codelyzer@6.0.1

# This is the commit message apache#7:

npm i ngx-clipboard@11.1.5

# This is the commit message apache#8:

npm i tsickle@0.32.1

# This is the commit message apache#9:

restore rxjs-compat@6.0.0-rc.0 to fix rxjs issue at npm run build

# This is the commit message apache#10:

WIP state with new Reactive Form

# This is the commit message apache#11:

Revert "WIP state with new Reactive Form"

This reverts commit f9e2e37.
micahstubbs added a commit to micahstubbs/helix that referenced this pull request Jun 1, 2022
parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654124422 -0700

# This is a combination of 2 commits.
# This is the 1st commit message:

parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654124376 -0700

ng update @angular/cli --from=5.2.11 --to=6 --migrate-only

npm i -D rxjs-tslint, add rxjs specific linting rules, add lint:tslint alternative linting script

npm install @angular/cdk@6.1.0

# This is the commit message apache#2:

npm install @angular/{animations,cdk,common,compiler,compiler-cli,core,forms,material,platform-browser,platform-browser-dynamic,platform-server,router}@6.1.0

# This is the commit message apache#5:

npm i ajv@6.9.1

# This is the commit message apache#6:

npm i codelyzer@6.0.1

# This is the commit message apache#7:

npm i ngx-clipboard@11.1.5

# This is the commit message apache#8:

npm i tsickle@0.32.1

# This is the commit message apache#9:

restore rxjs-compat@6.0.0-rc.0 to fix rxjs issue at npm run build

# This is the commit message apache#10:

WIP state with new Reactive Form

# This is the commit message apache#11:

Revert "WIP state with new Reactive Form"

This reverts commit f9e2e37.
micahstubbs added a commit to micahstubbs/helix that referenced this pull request Jun 1, 2022
parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654124891 -0700

# This is a combination of 2 commits.
# This is the 1st commit message:

parent ed990b8
author Micah Stubbs <micah.stubbs@gmail.com> 1651701734 -0700
committer Micah Stubbs <micah.stubbs@gmail.com> 1654124832 -0700

ng update @angular/cli --from=5.2.11 --to=6 --migrate-only

npm i -D rxjs-tslint, add rxjs specific linting rules, add lint:tslint alternative linting script

npm install @angular/cdk@6.1.0

# This is the commit message apache#2:

npm install @angular/{animations,cdk,common,compiler,compiler-cli,core,forms,material,platform-browser,platform-browser-dynamic,platform-server,router}@6.1.0

# This is the commit message apache#5:

npm i ajv@6.9.1

# This is the commit message apache#6:

npm i codelyzer@6.0.1

# This is the commit message apache#7:

npm i ngx-clipboard@11.1.5

# This is the commit message apache#8:

npm i tsickle@0.32.1

# This is the commit message apache#9:

restore rxjs-compat@6.0.0-rc.0 to fix rxjs issue at npm run build

# This is the commit message apache#10:

register proxy.conf.json in ng serve builder in angular.json

# This is the commit message apache#11:

WIP state with new Reactive Form

# This is the commit message apache#12:

Revert "WIP state with new Reactive Form"

This reverts commit f9e2e37.
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

4 participants