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

Refactor MigrationListener API #15071

Merged
merged 1 commit into from Oct 2, 2019
Merged

Conversation

@mdogan
Copy link
Member

mdogan commented May 22, 2019

With new listener, an event is published when a new migration process starts
and another event when migration is completed. Those events include stats
about migration process; start time, planned migration count, completed migration count etc.

Additionally, on each replica migration, both for primary and backup replica migrations,
a migration event is published. This event includes partition-id, replica-index and
migration progress stats.

https://hazelcast.atlassian.net/wiki/spaces/EN/pages/1749680489/Refactor+MigrationListener+API+Design

@mdogan mdogan added this to the 4.0 milestone May 22, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch 2 times, most recently from 85554c4 to c91dc36 May 27, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch from c91dc36 to f67158a Jun 17, 2019
@mdogan mdogan marked this pull request as ready for review Jun 17, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch from f67158a to a1c15ca Jun 18, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch from a1c15ca to c750206 Jun 20, 2019
@mmedenjak mmedenjak requested a review from devozerov Jul 23, 2019
Copy link
Member

devozerov left a comment

@mdogan @mmedenjak I reviewed the code and I have only recommendations about public API.

  1. Speaking of general terms, currently, we use the migration process (whole) and migration (replica). IMO migration (whole) and replica migration (replica) might be clearer to the user, as it set's an explicit parent-child relationship between stages and doesn't require the user to learn what the "process" is
  2. I am not sure if MigrationListener.migrationFailed method is needed since MigrationEvent already has success flag
  3. Additional information about failure cause in MigrationEvent (e.g. error string, member ID where an error occurred) might be useful for debugging. This would help the user to easier understand the next steps
  4. I would propose to merge MigrationPlan and MigrationProgress into a single interface
    MigrationStatus (or MigrationEvent) to minimize the number of interfaces the user needs to learn.

Taking all this in count, resulting API could be as simple as this:
MigrationEvent - occurs on global start and stop
ReplicaMigrationEvent - occurs when replica migration finished

interface MigrationListener {
    void migrationStarted(MigrationEvent);
    void migrationCompleted(MigrationEvent);
    void replicaMigrationCompleted(ReplicaMigrationEvent);
}
@mdogan

This comment has been minimized.

Copy link
Member Author

mdogan commented Jul 25, 2019

Thanks for the review @devozerov.

  1. My initial naming was to use migration and replica migration. But I wasn't sure, then I changed them to migration process and migration. I can change them back. This is a good feedback.

  2. I tend to keep MigrationListener.migrationFailed(..), since it has a better visibility than a flag inside the event object. When MigrationListener.migrationCompleted(..) is called, users may assume migration succeeded.

  3. I will look into that.

  4. Make sense.

@mdogan mdogan force-pushed the mdogan:migration-listener branch 2 times, most recently from e16eef5 to f9c7c26 Jul 25, 2019
@mmedenjak mmedenjak self-requested a review Sep 4, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch from f9c7c26 to bacdf04 Sep 5, 2019
@mdogan mdogan force-pushed the mdogan:migration-listener branch from bacdf04 to de6815a Sep 30, 2019
@mdogan mdogan requested a review from hazelcast/clients as a code owner Sep 30, 2019
With new listener, an event is published when a new migration process starts
and another event when migration is completed. Those events include stats
about migration process; start time, planned migration count, completed migration count etc.

Additionally, on each replica migration, both for primary and backup replica migrations,
a migration event is published. This event includes partition-id, replica-index and
migration progress stats.
@mdogan mdogan force-pushed the mdogan:migration-listener branch from de6815a to 2392b71 Oct 1, 2019
@devozerov devozerov self-requested a review Oct 1, 2019
Copy link
Contributor

mmedenjak left a comment

Nice, added one more minor comment for consideration

@mdogan

This comment has been minimized.

Copy link
Member Author

mdogan commented Oct 1, 2019

Thanks for the reviews @devozerov and @mmedenjak.

@mdogan mdogan merged commit 81c51e8 into hazelcast:master Oct 2, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mdogan mdogan deleted the mdogan:migration-listener branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.