appengine icon indicating copy to clipboard operation
appengine copied to clipboard

datastore: allow pointer to struct type fields

Open jjhuff opened this issue 6 years ago • 9 comments

One of Google's other golang datastore clients has implemented a feature I find rather handy: allowing pointers to struct fields (as well as some other stuff).

https://github.com/GoogleCloudPlatform/google-cloud-go/commit/0e0ec87d77f27fbf630f4599b7134faba020634f

Unfortunately, that library's datastore code doesn't work on AppEngine standard (that is seriously confusing, BTW)

I'd be happy to look at porting this over (and/or trying to sync up), but before I invest the time, I'd like to know if that makes sense for this project. Is there a future for this code base? Thanks!

jjhuff avatar Feb 14 '18 18:02 jjhuff

I support this idea.

When I brought this up back in 2015 the reply was that this would not be implemented by Google because you could achieve this with PropertyLoadSaver. While true, I still think there's a lot of value in having it supported out of the box.

I could provide some help reviewing/testing the changes.

Unfortunately this repo is in some sort of legacy low priority mode. Just looking at the open issues will give a good overview of the ghost town we're in.

That said, if we do have a backwards compatible PR with tests, it might be possible to get it merged by contacting specific people. General targeting of "Google people" usually ends up in an example of the bystander effect where nobody helps.

xStrom avatar Feb 15 '18 13:02 xStrom

@jjhuff Does that library not work on AppEngine Standard? I'm 100% sure I use the cloud.google.com/go/datastore package in my project as I mix between the Standard and Flex runtimes. The only thing is the disconnect locally with the built-in datastore emulator on the dev app server so I exclusively utilize a "test" project and bootstrap accordingly. I guess I could have also just used the datastore emulator as part of the gcloud cli.

Here's how I get the datastore client:

package models

import (
	"net"
	"os"
	"time"

	"cloud.google.com/go/datastore"
	"go.opencensus.io/plugin/grpc/grpctrace"
	"golang.org/x/net/context"
	"google.golang.org/api/option"
	"google.golang.org/appengine"
	"google.golang.org/grpc"
)

func init() {
        // This can probably be done using the ENV feature of app.yaml
	if appengine.IsDevAppServer() {
		os.Setenv("DATASTORE_PROJECT_ID", "dev-project")
	} else {
		os.Setenv("DATASTORE_PROJECT_ID", "main-project")
	}
}

func getDatastoreClient(ctx context.Context) (*datastore.Client, error) {
	var options []option.ClientOption

	options = append(options, option.WithGRPCDialOption(grpc.WithStatsHandler(grpctrace.NewClientStatsHandler())))
	if appengine.IsDevAppServer() {
		// Such a ***** hack to work locally :(
		opt := option.WithGRPCDialOption(
			grpc.WithDialer(
				func(addr string, timeout time.Duration) (net.Conn, error) {
					return (&net.Dialer{Cancel: ctx.Done()}).Dial("tcp", "datastore.googleapis.com:443")
				},
			),
		)
		options = append(options, opt)
	}
	return datastore.NewClient(ctx, "", options...)
}

someone1 avatar Feb 21 '18 15:02 someone1

@someone1 two questions:

  1. How do you manage to insert to task queue transitionally within datastore transaction? E.g. insert/update/delete few DB records and insert few queue tasks withing same transaction.

  2. Where do you pass credentials for datastore connection in case of IsDevAppServer() == true?

trakhimenok avatar Feb 21 '18 16:02 trakhimenok

Here you go:

  1. I don't, I guess there's no feature parity for this outside of the standard environment, you also don't have access to the taskqueue API outside of the standard environment (yet - Cloud Task API is in alpha but even here they don't bring this functionality). With some manual handling, I'm sure a workaround can be found here like storing datastore entires for the tasks along with the transaction and enqueuing a normal task to try and read for those entries and enqueue them, maybe even have a cron job that runs periodically to catch anything that was missed (not a fully vetted idea off the top of my head)

  2. I start my dev_appserver with the appidentity_email_address and appidentity_private_key_path flags, along with the env_var flag setting the GOOGLE_APPLICATION_CREDENTIALS variable accordingly (the first two are used for my python code and can probably be ignored, the env_var is probably all that needs to be set.)

someone1 avatar Feb 21 '18 18:02 someone1

I should note that these two libraries are not binary compatible. That is to say, they serialize certain entities in different ways. So if you have some complex entities saved with github.com/golang/appengine you can't load them with cloud.google.com/go/datastore. Also, because these two libraries don't have a goal of compatibility, even if you successfully migrate from one to the other, you better not be planning on switching between the two on a regular basis, because who knows what might break.

xStrom avatar Feb 23 '18 08:02 xStrom

I do think its a one way migration, but as the new package should be a functional improvement over the old (except for transactional tasks), there shouldn't be any reason to move to the old package.

In my experience, the new package does serialize differently than the old, but it is able to read entities written from the old datastore package without issue. I'm not sure what complex entities you might have that it can't support (I don't doubt there is such a thing), but I think it's worth trying to see if it does work if you haven't actually done so yet. You can also probably write code to read an entity using the old datastore package and rewrite them with the new fairly trivially if this is an issue as a migration path.

If you have some examples of what won't work, could you please share them?

someone1 avatar Feb 23 '18 20:02 someone1

I seem to recall that, at some point, there used to be a performance penalty when using cloud.google.com/go/datastore from GAE Standard (probably due to needing to use it's socket API), but I'm not sure how much of an issue that is anymore. @someone1, have you seen this?

Ultimately, I might just go ahead and migrate to GAE flexible since it seems like standard is atrophying.

jjhuff avatar Feb 24 '18 01:02 jjhuff

Unfortunately I don't remember the specifics. It has been months since I looked into the code. I think they changed how nesting of structs is serialized. Maybe this?

In any case, the more important point is that they don't have a goal of compatibility. So the specifics of what won't work can change any moment.

xStrom avatar Feb 24 '18 13:02 xStrom

@jjhuff performance is more consistent but since I lost memcache during the transition (we were using nds), performance took a small hit (but one worth the switch). Hopefully nds updates to the new datastore package once the memcache API comes out. I agree with the flexible sentiment, it's a shame what's happening with the standard runtime. I have work arounds for everything except the lack of a search API so I can't move away from standard completely.

@xStrom this is exactly what I noticed but experience no issues from. It serializes differently but can still read the old way. Once you're on the new package I don't see why any compatibility break with the old should matter but I won't assume to understand your use case.

I hope my experience helps anyone looking to make the jump, I think it should be fine for most use cases and should probably be used for any new projects.

someone1 avatar Feb 24 '18 22:02 someone1