Skip to content

Two patches to scheduler - #682

Closed
herberteuler wants to merge 2 commits into
nathanmarz:masterfrom
herberteuler:master
Closed

Two patches to scheduler#682
herberteuler wants to merge 2 commits into
nathanmarz:masterfrom
herberteuler:master

Conversation

@herberteuler

Copy link
Copy Markdown
Contributor

There are two patches here:

  1. There seems to be an erroneous structure in default-schedule: invoking to schedule-topologies-evenly should only be done after freeing all bad-slots for topologies that needs re-scheduling.
  2. SchedulerAssignmentImpl currently uses HashMap, which does not preserve entries order. This can cause unnecessary and unexpected re-scheduling when nimbus converts executor->node+port between its internal Clojure maps and SchedulerAssignmentImpl's Java maps back and forth.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

topology-id is declared in doseq, while you are referencing topology-id out of doseq

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I see.

@xumingming

Copy link
Copy Markdown
Collaborator

@herberteuler any update on this PR?

@herberteuler

Copy link
Copy Markdown
Contributor Author

The LinkedHashMap change has been in the branch for our testing cluster for a while, and it has fixed the unexpected rescheduling issue there. We are planning to incorporate it into our next installation on the production cluster.

@xumingming

Copy link
Copy Markdown
Collaborator

@herberteuler can you describe a case which have issues with current implementation, while is fixed by your patch?

@herberteuler

Copy link
Copy Markdown
Contributor Author

I'll make a new PR later, with more detailed descriptions. Sorry for the inconvenience.

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