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

Create metadata db interface #298

Merged
merged 10 commits into from
Jul 2, 2018
Merged

Create metadata db interface #298

merged 10 commits into from
Jul 2, 2018

Conversation

feclare
Copy link
Contributor

@feclare feclare commented Jun 13, 2018

It also adds:

  • new memory db backend for metadata
  • tests for the new interface(memory and mongo)

Fixes #290

@feclare feclare requested a review from tsconn23 June 13, 2018 14:05
@tsconn23 tsconn23 added the hold Intended for PRs we want to flag for ongoing review label Jun 13, 2018
@tsconn23
Copy link
Member

Review is pending. We do NOT want to pull this in until after the California release.

@jduranf
Copy link
Contributor

jduranf commented Jun 13, 2018

@feclare nice work!
Some comments:

  • /core/db/xxx/client.go should be split into two files client.go and data.go. The first one should have all common code, the second one should have code relative to core-data
  • makeTimestamp() should be moved in /core/db/db.go, and be used in memory and influx
  • Connect() and CloseSession() in /core/db/mongo/metadata.go should be moved to client.go
  • In core-data we have /core/data/clients/database-client.go, and in core-metadata we have /core/metadata/interfaces/dbOps.go. We should unify the file name (and path) criteria.
  • /core/domain/enums/database.go and /core/domain/enums/database_test.go should be removed. Only is used the DATABASE enumeration, which can be moved into dbOps.go (or wherever name you picked), and the enumeration name maybe should be changed to DatabaseType, like is done in /core/data//clients/database-client.go
  • Database type should be selected by configuration, no hard coded. But maybe we can leave this point for another PR

It's a big PR and I didn't check everything. If it's fine, I will continue after you fix some of the points I commented.

@feclare
Copy link
Contributor Author

feclare commented Jun 14, 2018

@jduranf Thanks for the review:

  • /core/db/xxx/client.go I agree that having a client.go, data.go and metadata.go in each backend makes the code nices. Will do
  • makeTimestamp is also used in support/logging. Maybe moving it to edgex-go/internal could be more reasonable. @tsconn23 what do you think?
  • Move Connect and CloseSession in mongo to client.go. Will do
  • I mostly agree, but I have some concerns about having the same package name in several places (core/data/interfaces, core/metadata/interfaces,...) as we will need to rename the package when importing more than one interface package in a go file. Maybe having core/data/datainterface and core/metadata/metadatainterface could be an option.
  • I agree the DATABASE enumeration and the backend used in each microservice needs to be designed again after this PR. metadata db backend could be selected by configuration in this PR

@tsconn23
Copy link
Member

@feclare

  • makeTimestamp -- I think I'd just go with @jduranf suggestion for now and move it to db.go. If we moved it to a globally available internal package we might have to consider making it an interface (Timestamper) as I don't think we'd want to establish a practice of having global functions. I can just see un-mockable dependencies creeping in if we go down that road. We can re-evaluate later, but keep the effort constrained for now. You've got enough here ;-)

  • Interfaces (data/metadata) -- The idea is those interfaces are internal to the given package so I can't see a case where some other package would have to import them both. Because of that, the core-data DBClient could actually be lower-cased to encapsulate it within that package. Same for metadata. What you might want to consider is simply having a db_client.go right under core/data and core/metadata without the interfaces directory. That way the interface is right under the package using it.

@feclare
Copy link
Contributor Author

feclare commented Jun 18, 2018

@tsconn23
The data and metadata interfaces are used in core/db/test. There are functions used in the tests of the db implementations, and they are using the data and metadata interfaces. This way we can use the same functions to test all the db implementations.
I will try to have a interfaces in data and metadata and we can continue iterating the solution from there.

@JPWKU
Copy link
Contributor

JPWKU commented Jun 19, 2018

recheck

// DeviceResources []models.DeviceObject `bson:"deviceResources"`
// Resources []models.ProfileResource `bson:"resources"`
// Commands []mgo.DBRef `bson:"commands"` // List of commands to Get/Put information for devices associated with this profile
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not remove this structure?

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

return err
}
s := m.session.Copy()
defer s.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

In event.go isn't used m.session.Copy() and the other related stuff.
Which is the best way to get the values? Maybe the criteria should be unified in another PR

jduranf
jduranf previously approved these changes Jun 20, 2018
Copy link
Contributor

@jduranf jduranf left a comment

Choose a reason for hiding this comment

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

LGTM

@tsconn23
Copy link
Member

tsconn23 commented Jun 29, 2018

@feclare Alright, we're getting closer to the time when we want to bring this in. If you don't mind, rebase the branch to get rid of the "out of date" message below. Then I'd like to do one last review.

feclare added 10 commits July 2, 2018 09:50
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Moved the core-data db functions to core/db

Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Also adding tests for metadata db backends (mongo and memory at the moment)

Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Also made some types privates

Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Also made all databases to use this functions when obtaining a timestamp

Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
Signed-off-by: Federico Claramonte <fede.claramonte@caviumnetworks.com>
@feclare
Copy link
Contributor Author

feclare commented Jul 2, 2018

@tsconn23 Done.

@tsconn23 tsconn23 removed the hold Intended for PRs we want to flag for ongoing review label Jul 2, 2018
@tsconn23
Copy link
Member

tsconn23 commented Jul 2, 2018

@feclare I've tested this PR using the device-virtual service as well as via running the blackbox tests locally. Everything passes. @tonyespy @SteveOss I also reviewed the latest Go device service SDK to see if it imported any of the refactored database logic directly and it does not, so I don't expect any adverse effects there. I'm going to pull this in. There are a couple of issues I'm going to create and associate to this PR for subsequent work. They're just minor things based on final code review. Nice work!

Copy link
Member

@tsconn23 tsconn23 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

4 participants