Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

refactor(split): move replica split functions into replica_split_manager class #624

Merged
merged 6 commits into from
Sep 17, 2020

Conversation

hycdong
Copy link
Contributor

@hycdong hycdong commented Sep 11, 2020

In previous implmentation, split varieties and functions are defined in replica.h, this pull request adds a new class replica_split_manager and moves them into this class.

  • move split varieties and functions definition from replica.h to split/replica_split_manager.h
  • move split function implementation from replica_split.cpp to split/replica_split_manager.cpp
  • move split unit test from test/replica_split_test.cpp into split/test/replica_split_test.cpp

By the way, code of split will be deep refactored in further pull request, this pull request only does basic refactor to add replica_split_manager class.

@hycdong hycdong marked this pull request as ready for review September 11, 2020 05:46
@hycdong hycdong changed the title refactor: move replica split functions into replica_split_manager class refactor(split): move replica split functions into replica_split_manager class Sep 11, 2020
@neverchanje
Copy link
Contributor

move split varieties and functions definition from replica.h to partition_split/replica_split_manager.h

Name the directory to split. "partition_split" is too long.

@hycdong
Copy link
Contributor Author

hycdong commented Sep 11, 2020

move split varieties and functions definition from replica.h to partition_split/replica_split_manager.h

Name the directory to split. "partition_split" is too long.

Done


#pragma once

#include "replica/replica.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

why didn't you name it partition_split_manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class name is replica_split_manager, showing the function is partition_split, and it is used for replica, just like replica_bulk_loader.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I saw some comments write Partition Split, this will make someone new confused with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suitable class name may be replica_partition_split_manager, but it is a little long, so I name it replica_split_manager, split is the abbreviation of partition split. Could you please tell me what is your confusion raised by partition split?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to give it a uniform name. or someone without a relevant knowledge background may feel confused with what's the difference between partition split and replica split

Copy link
Contributor

Choose a reason for hiding this comment

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

Partition Split is the user-facing word. Inside the implementation, child/parent replica both do the split job, in replica level. So replica_split_manager is more accurate.

Futhermore, it would be better if child and parent could be separated to different classes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants