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

chore(worker): unify mapper receiver names #8740

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

hezhizhen
Copy link
Contributor

Problem

mapper methods have different receiver names: m and mw.

Solution

Unify receiver names for mapper.

Simplify some code btw.

@dgraph-bot dgraph-bot added area/core internal mechanisms go Pull requests that update Go code labels Mar 8, 2023
worker/backup_handler.go Show resolved Hide resolved
worker/draft.go Outdated
@@ -192,13 +191,12 @@ func (n *node) startTaskAtTs(id op, ts uint64) (*z.Closer, error) {
}
case opSnapshot, opPredMove:
for otherId, otherOp := range n.ops {
if otherId == opRollup {
Copy link
Member

Choose a reason for hiding this comment

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

I understand that the modified code is more Go like but the original code seems more readable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if and else are rather straightforward, which makes the code seem more readable. Go's style is a few small tweaks without loosing the readability.

In this case, I first change the order of if and else:

if otherId != opRollup { return nil, errors.Errorf("operation %s is already running", otherId) }
else {
    // Remove from map and signal the closer to cancel the operation.
    delete(n.ops, otherId)
    otherOp.SignalAndWait()
}

Then I remove the unnecessary else:

if otherId != opRollup { return nil, errors.Errorf("operation %s is already running", otherId) }

// Remove from map and signal the closer to cancel the operation.
delete(n.ops, otherId)
otherOp.SignalAndWait()

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I get that the code is equivalent. What I don't like about the modified code is that it does delete(n.ops, otherId) and I don't know what otherId is until I see the if condition. This seems a bit convoluted to read to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see... Since you read the function in a linear way (top down), you see the if condition first, and then you see delete(n.ops, otherId). Although it is not wrapped in a else block, the return in the if block indicates it.

I suppose that you are not accustomed to this style because you are more familiar with other languages. It's okay to revert it if it really bothers you.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we could rewrite the code like this. This would feel more explicit and clear.

if otherId != opRollup { return nil, errors.Errorf("operation %s is already running", otherId) }

// Remove from map and signal the closer to cancel the operation.
delete(n.ops, opRollup)
otherOp.SignalAndWait()

Copy link
Member

Choose a reason for hiding this comment

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

I have full confidence in the code, and you are right that it would work correctly. My challenge is with the situation when we make changes to this code in the future and we may miss out on this fact that the delete happens only for a particular case because that is not immediately obvious. I hope that clears things up.

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 initial version is:

if otherId == opRollup {
    // Remove from map and signal the closer to cancel the operation. 
    delete(n.ops, otherId) 
    otherOp.SignalAndWait()
} else {
    return nil, errors.Errorf("operation %s is already running", otherId) 
}

It is rather straightfoward.

Go's version is:

if otherId != opRollup {
    return nil, errors.Errorf("operation %s is already running", otherId) 
}

// Remove from map and signal the closer to cancel the operation. 
delete(n.ops, otherId) 
otherOp.SignalAndWait()

Here is your suggestion:

if otherId != opRollup {
    return nil, errors.Errorf("operation %s is already running", otherId) 
}

// Remove from map and signal the closer to cancel the operation. 
delete(n.ops, opRollup) 
otherOp.SignalAndWait()

It is to make sure that the deleted key is opRollup (correct me if I'm wrong).

Let's think about adding more handling logic for key opBackup.

  • initial version: add an else if block
  • Go's version: use switch and handle opRollup and opBackup cases respectively (my suggestion)
  • your suggestion: just like Go's version

Actually I'm not sure if I'm simplifying it or decreasing its readability. I'm accustomed to Go's style and it does reduces complexity. Last but not the least, we can add unit tests to ensure its correctness.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that. If you write unit test for it, then I don't care what design you choose. Because if I make a change later on that is messing up something, my unit tests will fail. I would suggest that get this PR merged without any change here and raise a separate PR with this change and tests for 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.

Let's do that. If you write unit test for it, then I don't care what design you choose. Because if I make a change later on that is messing up something, my unit tests will fail. I would suggest that get this PR merged without any change here and raise a separate PR with this change and tests for it.

Okay. I'll do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL @mangalaman93

@hezhizhen
Copy link
Contributor Author

@mangalaman93 The code coverage test failed. Could you please re-run it?

@mangalaman93 mangalaman93 merged commit c8ae352 into hypermodeinc:main Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core internal mechanisms go Pull requests that update Go code
Development

Successfully merging this pull request may close these issues.

4 participants