Support PropertyLoadSaver Interface #69
Conversation
|
I'll find time for review later this week. |
|
How is the progress? |
|
I apologize, this got deprioritized and somewhat forgotten. Good to know that there's active interest! I will make an effort to get this functionality tested & merged. @yuichi1004 are you still interested in helping make this happen? There are merge conflicts right now, I think it's these two PRs that got merged after you submitted your changes: #71 & #73. If @yuichi1004 wants to help and fixes the merge conflicts, I will review/test it. If not, it may take a bit longer but I will do the necessary work myself. |
|
@xStrom @hamakn I have one failure test but I do believe this is irrelevant to this change and the bug of App Engine dev server. |
|
Using |
|
I think it should call https://github.com/yuichi1004/goon/blob/3d84cd76c6d15bf622305c3e8e9623603749fbb0/goon.go#L454-L458 if s, present := g.cache[m]; present {
if vi.Kind() == reflect.Interface {
// Load() is needed here?
vi = vi.Elem()
}
reflect.Indirect(vi).Set(reflect.Indirect(reflect.ValueOf(s)))Is this just as you intended? It causes entities in |
|
Hi, @yuichi1004 I tried this feature, but when reproduction codepackage goon
import (
"testing"
"google.golang.org/appengine/aetest"
"google.golang.org/appengine/datastore"
"google.golang.org/appengine/memcache"
)
type Value struct {
ID int64 `datastore:"-" goon:"id"`
String string `datastore:"string"`
}
func (v *Value) Save() ([]datastore.Property, error) { return datastore.SaveStruct(v) }
func (v *Value) Load(p []datastore.Property) error { return datastore.LoadStruct(v, p) }
func TestPLS(t *testing.T) {
c, done, err := aetest.NewContext()
if err != nil {
t.Fatal(err)
}
defer done()
g := FromContext(c)
// Put
v := Value{String: "test"}
k, err := g.Put(&v)
if err != nil {
t.Fatal(err)
}
g.FlushLocalCache()
// Get from datastore
v1 := Value{ID: v.ID}
if err := g.Get(&v1); err != nil {
t.Fatal(err)
}
t.Log("from datastore:", v1)
// Get from memcache
s, err := memcache.Get(c, MemcacheKey(k))
if err != nil {
t.Fatal(err)
}
var v2 Value
if err := deserializeStruct(&v2, s.Value); err != nil {
t.Fatalf("could not deserialize `%v`: %s", s.Value, err) // err == errCacheFetchFailed
}
t.Log("from memcache:", v2)
} |
|
@delphinus @daisuzu |
|
@daisuzu Well, that's a silly mistake... 005ae6a |
yuichi1004
force-pushed
the
topic/support_property_load_saver
branch
from
7b65b06
to
005ae6a
Compare
|
Ah, I see... |
|
@delphinus I am still looking into that topic. We can simplify all of those complexity if we fully migrated to PropertyList based implementation. If underlying core logic were written in PropertyList, then PLS struct and non-PLS struct can work on the same caching logic. But for now, I will go with the easy approach since this is a bigger discussion than original topic. |
|
Agree. If it contains PLS process for local cache, it should encode entities into cache instead of the raw structs as current logic does. It makes some performance impact, and so I also think your plan is good for this time. |
|
I am still working on avoiding local cache for PLS struct. I got weird error related to re-using deserialization decoder. I appreciate if anybody can provide the fix, otherwise I will work on next weekend. |
entity.go
Outdated
| serializeStructMetaData(finalBuf[1:], smd) // Serialize the metadata | ||
| copy(finalBuf[finalBufSize-bufSize:], se.buf.Bytes()) // Copy the actual data | ||
| if ls, ok := src.(datastore.PropertyLoadSaver); ok { | ||
| se.buf.Write([]byte{serializationStatePropertyList}) |
There was a problem hiding this comment.
We should use the WriteByte method to skip the slice allocation.
goon_test.go
Outdated
| } | ||
|
|
||
| // Make sure that MigrationC implements datastore.PropertyLoadSaver | ||
| var _, _ datastore.PropertyLoadSaver = &MigrationPlsA{}, &MigrationPlsB{} |
There was a problem hiding this comment.
The MigrationC reference in the comment seems outdated. Should change the wording to something like: Make sure that these implement datastore.PropertyLoadSaver.
goon_test.go
Outdated
| name string | ||
| ignoreFieldMismatch bool | ||
| src, dst MigrationEntity | ||
| }{ |
There was a problem hiding this comment.
We should still test with IgnoreFieldMismatch on & off with all the variants, not just NormalCache -> NormalCache. Writing all of these out can get too verbose (8 cases), so we could still do a loop for i := 0; i < 2; i++ { like before and inside the loop append the 4 different types to the testcase slice, with IgnoreFieldMismatch set based on the loop variable.
There was a problem hiding this comment.
Fixed dd18dfd...1c5058d
That doesn't sound right to me. The local cache should basically be just a pointer. [1] Thus, with a reference-based cache, whether we load the serialized data from datastore or memcache, we'll execute the [1] There has been some previous discussion over reference-based vs. copy-based local cache in PR #27. As the years have passed I've become even more convinced that a referenced-based cache is superior. Unfortunately I haven't gotten around to writing tests for this yet, so there may be some edge cases where shallow copies are being produced unintentionally. However there definitely isn't a proper copy-based system. |
|
I've now gone over the changes once. I pointed out some basic issues. I also have some more fundamental thoughts on the implementation strategy. I'll need a bit more time to think this over and then I'll comment on it. |
|
@xStrom thanks for your review and comment. For the discussion about reference-based cache over copy-based cache, I actually like reference-based cache. My assumption was some users use @delphinus correct me if you need |
|
@xStrom I understand. This behavior is correct only when type Entity {
ID int64 `datastore:"-" goon:"id"`
Foo string
Unix int64
CreatedAt time.Time `datastore:"-"`
}
func (e *Entity) Load([]datastore.Property), error) {
_ = datastore.LoadStruct(e)
e.CreatedAt = time.Unix(e.Unix, 0)
return nil
}
func (e *Entity) Save() ([]datastore.Property, error) {
e.Unix = e.CreatedAt.Unix()
return datastore.SaveStruct(e)
}The code above runs good with func (e *Entity) Load([]datastore.Property), error) {
_ = datastore.LoadStruct(e)
e.CreatedAt = time.Unix(e.Unix, 0)
e.Foo = e.Foo + " Loaded!!" // This breaks symmetry
return nil
}
func (e *Entity) Save() ([]datastore.Property, error) {
e.Unix = e.CreatedAt.Unix()
e.Foo = e.Foo + " Saved!" // This breaks symmetry
return datastore.SaveStruct(e)
}But how about above? They are both inverse, but have side effects. With test log (failed)Yes, this is not much practical. But someone may need simply hooks there (as @yuichi1004 says) and he/she might be at a loss this behavior that differs from This cannot be solved easily and that “side effectable” |
|
Thanks for the test case, it illustrates an interesting case. In my previous comment I talked about how What happens is that On a somewhat related topic, of course there are still cases where if you |
|
@xStrom @delphinus Thanks for your discussion. Let me clarify the action items
|
|
Speaking broadly, I think having a unified strategy for structs/PLS is superior. It would be fewer algorithms to reason about and probably less chance for bugs to be introduced in the future when changes are made. It would also allow us to fully support migrations between types. As opposed to the current situation where we can't load a PropertyList-based cache into a struct. Then there's still the question of which type of unified strategy to use. Depend more on the datastore package and do everything with Intuitively I think the correct solution is a hybrid of these two strategies, though leaning more towards goon's custom serialization. However this is a non-trivial undertaking. There's a saying that perfect is the enemy of good. I think that applies here as well. We should continue with the current split strategy that we have in this PR. We only need some smaller changes, including a few that I'll comment on soon. Then we can get this merged as an initial working version. Later on I'll open up another PR for work towards a unified solution, which can have its own schedule and won't hold back this PR. [1] One scenario where just encoding [2] Really old goon versions actually just used |
| type nilType struct { | ||
| GoonNilValue string | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's cut down on allocations and create a reusable value of this: var nilValue = nilType{}.
entity.go
Outdated
| gob.Register(seBoot.v13) | ||
| gob.Register(seBoot.v14) | ||
| gob.Register(seBoot.v15) | ||
| gob.Register(nilType{}) |
There was a problem hiding this comment.
Use the new nilValue instead of allocating a new one with nilType{}.
entity.go
Outdated
| for i := 0; i < len(props); i++ { | ||
| v := reflect.ValueOf(props[i].Value) | ||
| if v.Kind() == reflect.Ptr && v.IsNil() { | ||
| props[i].Value = nilType{} |
There was a problem hiding this comment.
Use the new nilValue instead of allocating a new one with nilType{}.
entity.go
Outdated
| } | ||
| for i := 0; i < len(props); i++ { | ||
| nilVal := nilType{} | ||
| if props[i].Value == nilVal { |
There was a problem hiding this comment.
Compare to the new nilValue instead of the local nilVal.
|
Reusing Currently we have: for _, tt := range testcase {
for _, IgnoreFieldMismatch = range []bool{true, false} {What I had in mind was something like this: for _, ifm := range []bool{true, false} {
testcase = append(testcase,
{
name: fmt.Sprintf("NormalCache -> NormalCache (IgnoreFieldMismatch:%v)", ifm),
ignoreFieldMismatch: ifm,
src: &MigrationA{Parent: parentKey, Id: 1},
dst: &MigrationB{Parent: parentKey, Identification: 1},
})
// .. and the 3 other appends as well, with ignoreFieldMismatch based on loop variable ifm
}This way we would keep the test configuration together and we would also have a fresh |
goon_test.go
Outdated
| migA = &MigrationA{Parent: parentKey, Id: 1} | ||
| if err := g.Get(migA); err != nil { | ||
| t.Errorf("Unexpected error on Get: %v", err) | ||
| for _, IgnoreFieldMismatch := range []bool{true, false} { |
There was a problem hiding this comment.
This loop declares a local scope variable called IgnoreFieldMismatch which hides the global one. I think the easiest fix is to change := to =.
There was a problem hiding this comment.
Well, that's a silly one...
Let me fix that quickly.
|
After we get this one final minor issue fixed ( |
|
Actually one other small thing. Could you also modify the // MemcacheKey returns key string of Memcache.
var MemcacheKey = func(k *datastore.Key) string {
// Versioning, so that incompatible changes to the cache system won't cause problems
return "g3:" + k.Encode()
} |
|
@xStrom I fixed what you pointed out. |
|
Great! I will merge this tomorrow (1 year anniversary of the PR 😄) unless someone objects. |
Introduction
This PR supports PropertyLoadSaver interface.
https://cloud.google.com/appengine/docs/standard/go/datastore/reference#hdr-The_PropertyLoadSaver_Interface
This PR should resolve #53.
Strategy
Please take a look at the code. Any comments, improvements, proposal of the different approach are welcome.