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

[DEMO: don't merge] Simple context #835

Closed
wants to merge 17 commits into from
Closed

Conversation

ethanfrey
Copy link
Contributor

Closes #822

  • weave.Context is now just context.Context
  • Do not store static app info there, that is all moved to weave.BlockInfo struct
  • Context carries authentication info, can be used for canceling (see Clean up store interface #823)

Important change is this interface:

type Handler interface {
	Check(ctx context.Context, info BlockInfo, store KVStore, tx Tx) (*CheckResult, error)
	Deliver(ctx context.Context, info BlockInfo, store KVStore, tx Tx) (*DeliverResult, error)
}

And all the code that had to be updated to use this interface. First 2-3 commits are only framework updates, rest is just fixing compile errors as a result of this API change

@ethanfrey ethanfrey requested review from husio and alpe June 27, 2019 19:24
Copy link
Contributor

@husio husio left a comment

Choose a reason for hiding this comment

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

This is a massive change 😮

I think removing weave.Context alias is a big improvement.

I think that removing variables from the context and providing a separate structure (BlockInfo) is a good call. It provides better visibility into the function input. It should be also harder to forget setting a value in the context. It also removes quite some panic calls 👍

I think context.Context is not actively used anywhere. Do we still need it, because we plan to use it for something soon?
I think we can remove context.Context from all notations. This is because context.Context is an interface. This means, that BlockInfo can implement it! 💪 In the current implementation, all functions require *BlockInfo which is good. Tomorrow if any function will need the cancellation functionality or only context.Context (not block info), then you can use *BlockInfo as the value, because it provides required functionality. What I mean is:

func Check(info *weave.BlockInfo, store weave.KVStore, tx weave.Tx) (*weave.DeliverResult, error) {
    return nil, foo(info)
}

func foo(ctx context.Context) error {
    return nil
}

*BlockInfo can be a superset of context.Context.

I really like to see InThePast, IntheFuture and others implemented as BlockInfo methods 💯

@@ -69,3 +71,22 @@ func SequenceID(n uint64) []byte {
binary.BigEndian.PutUint64(b, n)
return b
}

// BlockInfo returns a simple BlockInfo struct given just a height and optional time
Copy link
Contributor

Choose a reason for hiding this comment

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

🚓

BlockInfo returns a simple BlockInfo struct

simple is relative 😉

I wonder if instead of two functions BlockInfo and BlockInfoWithChain wouldn't it be better to provide only the more verbose one.
If this was a Python, only one function would be enough

def block_info(height, ts=None, chain_id="test-chain"):
    pass

In Go there are no default arguments. What we can do is similar to C, provide defaults for empty values.

// BlockInfo bla blabla.
// If nil time is provided, current wall clock time is used. If chain ID is not provided, default "test-chain" value is used.
func BlockInfo(height int64, ts *time.Time, chainID string) weave.BlockInfo {
    if ts == nil || ts.Zero() {
        ts = time.Now()
    }
    if chainID ==  "" {
        chainID = "test-chain"
    }
    // ...
}

You are allowing to use optional time value (...time.Time argument), which is a smart hack, but also a dead end for the API as changing such function notation is extremely hard.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is nice idea. Basically three ways to call and take default args

BlockInfo(59, &now, "my-chain")
BlockInfo(59, &now, "")
BlockInfo(59, nil, "")

I was a bit worried about *time.Time as we cannot inline calls here:

BlockInfo(59, &time.Now(), "foo-chain")

But when looking at the code, we always call time.Now() one place and reuse the variable in the calls. I will fix up weavetest with this improved syntax.

IsValidChainID = regexp.MustCompile(`^[a-zA-Z0-9_\-]{6,20}$`).MatchString
)

type BlockInfo struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

If BlockInfo is intended as not only block information container but as a general purpose request scope context, than maybe different name would be better. RequestScope or RequestContext 🙈

This implementation has all fields related to block information, except for the logger. Logger does not seem to fit this structure at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RequestScope is a nice idea.

Logger should be passed down somewhere, and sticking it in Context seemed like the uglier hack. Other solutions? I think we rarely if ever actually log anything, so maybe just remove it 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the logger not really fitting here. To pass it with the context was an anti pattern already (see https://dave.cheney.net/tag/logging). Instead let's set them explicitly with the constructors where needed.

Copy link
Contributor

@ruseinov ruseinov Jul 4, 2019

Choose a reason for hiding this comment

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

Agreed about logger.

IMHO:
Don't like the Scope naming, I don't think it's a name for a struct at all - gives me the wrong idea.
RequestScope sounds more like a constant to describe which scope we are in to execute some tasks that are intended for this scope. This is coming from a large application framework usage, so might be a matter of habit.

It also seems to me that we can't really operate with the term Request on blockchain, at least not with the current struct content as it gives the wrong idea.

I would much rather prefer something like ChainStatus or keep BlockInfo.

I can't think of a better name though, this one is pretty tough to name :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point with the block chain terminology.

How about State (used as weave.State)? This structure represents the current state of the application.

@ethanfrey
Copy link
Contributor Author

I think context.Context is not actively used anywhere. Do we still need it, because we plan to use it for something soon?

Even after this change, we still use it for the Authentication information, as those are added dynamically and have reader/writers in each module that touches it, and handle missing info gracefully, which are my main criteria for throwing data into Context.

With the other proposed DB and Bucket changes, you proposed plumbing context everywhere to allow for cancel/timeout, but also tracing and metric collection.

I dug into the use of Context for instrumentation some years ago in go api servers, and would love to get proper runtime instrumentation on weave - so we can see what functions take long, not just in benchmarks, but in real world cases. You can also only enable this collection on a few non-validator nodes, which just replay all actions and act as canaries to show bottlenecks, without adding the instrumentation overhead (and possibly security holes) to the consensus-critical validators.

@ethanfrey
Copy link
Contributor Author

Re: instrumentation (this is an aside, but why Context is cool), see #17 and add a comment or two... I think this is highly relevant as we approach production status

@ruseinov
Copy link
Contributor

ruseinov commented Jul 4, 2019

Implementation seems good to me, except for the logger.

And the default value idea is good too.

@husio
Copy link
Contributor

husio commented Jul 26, 2019

Resolving all those conflicts is pain. I am willing to do this, but before I would like to make sure that we are going to merge it. Otherwise no point in me updating this code.

@ruseinov
Copy link
Contributor

I think this is going to have to hang for a while as it touches too many packages.
Let's keep it for reference.

@ethanfrey
Copy link
Contributor Author

I think this is just reference now. Many of the changes will be undone after the comments above.

When you want to tackle this, I would just look at context.go and rename that and maybe use some of the code I wrote (with modifications). All other code to update all handlers, etc will be easier to redo from the master at time of a proper PR.

This PR is really just for reference and conversation, never to be merged

@ethanfrey ethanfrey changed the title Simple context [DEMO: don't merge] Simple context Jul 29, 2019
@kmw101 kmw101 closed this Aug 7, 2019
@husio husio deleted the spike-simple-context branch November 13, 2019 11:50
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.

Revisit weave.Context
5 participants