implement prepareForDataSynchronization func #51
Conversation
peng225
force-pushed
the
impl-data-sync-prepare
branch
6 times, most recently
from
5d1e66c
to
331f0c4
Compare
There was a problem hiding this comment.
@peng225 I've reviewed only implementation code and have not reviewed test code yet. I'll provide the remaining feedback later.
peng225
force-pushed
the
impl-data-sync-prepare
branch
from
6be7ae4
to
6ef1a3c
Compare
| func searchForDiffOriginMantleBackup( | ||
| backup *mantlev1.MantleBackup, | ||
| primaryBackups []mantlev1.MantleBackup, | ||
| secondaryBackupSet map[string]*mantlev1.MantleBackup, |
There was a problem hiding this comment.
| secondaryBackupSet map[string]*mantlev1.MantleBackup, | |
| secondaryBackupMap map[string]*mantlev1.MantleBackup, |
| DescribeTable("Search for the MantleBackup which is used for the diff origin", | ||
| func(backup *mantlev1.MantleBackup, | ||
| primaryBackups []mantlev1.MantleBackup, | ||
| secondaryBackupSet map[string]*mantlev1.MantleBackup, |
There was a problem hiding this comment.
| secondaryBackupSet map[string]*mantlev1.MantleBackup, | |
| secondaryBackupMap map[string]*mantlev1.MantleBackup, |
|
|
||
| testSecondaryMantleBackups := map[string]*mantlev1.MantleBackup{ | ||
| "test1": basePrimaryBackups[0].DeepCopy(), | ||
| "test2": basePrimaryBackups[1].DeepCopy(), |
There was a problem hiding this comment.
Could you tell me why testSecondaryMantleBackups includes "test2"? "test1", "test3", "test4", "test0", and "test2" are replicated in order by design.
There was a problem hiding this comment.
@peng225 Sorry, I should have explained what I meant more clearly. My point was, "test2" mon't be existed in the secondary k8s cluster because its snapID is higher than the "test0"'s one.
There was a problem hiding this comment.
I understand the design is as you explained.
I just believe that we should check every possible condition in the unit tests, even if those conditions are unlikly to occur when everything is functioning correctly.
This is just my personal opinion. I can also remove such unlikely test cases. Which do you think is better?
There was a problem hiding this comment.
I understood. It's not necessary to remove the test code. Instead, could you add a comment to the purpose of the existence of "test2" in the secondary cluster?
There was a problem hiding this comment.
@satoru-takeuchi
Sorry, but I reconcidered it, and have changed my mind.
I misunderstood that the exsistence of "test2" on the secondary cluster contributed to the higher code coverrage, but I realised it did not.
Let me remove "test2" on the second cluster,
("test2" should be in the primary cluster to check if we can skip the MantleBackup with the higher snapID.)
There was a problem hiding this comment.
Let me remove "test2" on the second cluster,
I added a comment.
8b3b868
Signed-off-by: Shinya Hayashi <[email protected]>
peng225
force-pushed
the
impl-data-sync-prepare
branch
from
8b3b868
to
06397d5
Compare
|
@satoru-takeuchi |
Signed-off-by: Shinya Hayashi [email protected]