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

[Persistence] Fix Persistence Module TODOs - Consolidate Postgres #149

Closed
10 tasks
andrewnguyen22 opened this issue Aug 4, 2022 · 3 comments · Fixed by #219
Closed
10 tasks

[Persistence] Fix Persistence Module TODOs - Consolidate Postgres #149

andrewnguyen22 opened this issue Aug 4, 2022 · 3 comments · Fixed by #219
Assignees
Labels
core Core infrastructure - protocol related persistence Persistence specific changes

Comments

@andrewnguyen22
Copy link
Contributor

andrewnguyen22 commented Aug 4, 2022

Objective

A general cleanup issue is needed to tackle TODO's and ensure the persistence module is usable/readable by consolidating PostgresContext and PostgresDb as part of #172.

Origin Document

Should follow issue-#128, issue-#105, issue-#147 and issue-#148 the persistence module is messier and more difficult to understand than the developers would want for organic external contribution.

Goals / Deliverables

  • Run `log.
  • Finish outstanding TODO's to ensure end-to-end functionality
  • Consolidate PostgresContext and PostgresDb like so:
type PostgresContext struct {
	height int64
	tx         pgx.Tx
	blockstore kvstore.KVStore	
}
  • Avoid exposing PostgresContext and/or PostgresDb for testing purposes

General issue checklist

  • Update the appropriate CHANGELOG
  • Update the README
  • If applicable, update the source code tree explanation
  • If applicable, add or update a state, sequence or flowchart diagram using mermaid
  • Update any relevant global documentation & references
  • Document small issues / TODOs along the way

Creator: @andrewnguyen22
Co-creator: @Olshansk

@andrewnguyen22 andrewnguyen22 added the persistence Persistence specific changes label Aug 4, 2022
@Olshansk Olshansk added the core Core infrastructure - protocol related label Aug 5, 2022
@Olshansk
Copy link
Member

Olshansk commented Aug 5, 2022

Outside of the context of this, just figured I'd share that after we do a team massive cleanup of all our TODO tags, we can also automate it via #144.

@andrewnguyen22 Please also attach issues to the V1 Dashboard Projects so we can see it all in one place

@andrewnguyen22
Copy link
Contributor Author

Good call ^

@Olshansk Olshansk added this to the V1 Persistence Foundation milestone Aug 12, 2022
andrewnguyen22 pushed a commit that referenced this issue Aug 15, 2022
…ts (Issue #128 && Issue #105) (#140)

# Objective
closes #128 
- Enable the lifecycle of `block` processing and production by creating a transactional architecture around persistence contexts instead of writing directly to the database on every operation.

closes #105  and closes #104
- Deprecate and remove all `PrePersistence` related code.

# Origin Document
### Background

#128
_Multiple persistence contexts enable a proper lifecycle for validators and full nodes as they maintain 'ephemeral' states in situations like block validation/processing and block production._

Pretty early on - it was discovered that though ephemeral states are needed, there seems to be little validity in having multiple `write` contexts in parallel - rather enable write contexts sequentially (only one at a time) and enable multiple read contexts. This is currently not strictly enforced, rather needs discussion on where to delegate this responsibility (Consensus, Utility, or Persistence module).

#105 
[[Persistence] V1 Persistence Foundation Integration Issue](#104)

# Goals / Deliverables
- [x] Deliver completed code
- [ ] Document the architecture and intended use
- [x] Test the transactional nature within the persistence module
- [x] Deprecation of the PrePersistence Module and all related code, docs, resources, etc...

## General milestone checklist
- [x] Update all the relevant CHANGELOGs
- [x] Update all the relevant READMEs
- [ ] Update the source code tree explanation
- [ ] Add or update a state, sequence or flowchart diagram using [mermaid](https://mermaid-js.github.io/mermaid/)
- [x] Create followup milestones + issues
- [x] Document small TODO along the way

## Follow-up Work

All follow-up work is being tracked in #149

---

## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] If applicable, I have made corresponding changes to related or global README
- [ ] If applicable, I have added added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [x] If applicable, I have added tests that prove my fix is effective or that my feature works

--- 

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
Olshansk added a commit that referenced this issue Aug 18, 2022
#165)

# Objective

Follow up on both critical and non-critical fixes to #140.

# Origin Document

#140 deprecated PrePersistence and implemented multiple contexts. This PR is a follow with some stylistic changes, improvements in the readability of the tests, the addition of unit tests, bug fixes in some SQL business logic, and implementation of the read context.

Followup work is being tracked in #149.

## Learnings to streamline workflow in the future
- Do not conflate non-critical stylistic changes as it leads to more conflicts down the road
- Implemented an automated "LocalNet" for easier testing
- Try to plan work ahead of time so we can work in smaller PRs / iterations

## Changelog / changes

**Main persistence module changes:**

- Split `ConnectAndInitializeDatabase` into `connectToDatabase` and `initializeDatabase`
  - This enables creating multiple contexts in parallel without re-initializing the DB connection
- Fix the SQL query used in `SelectActors`, `SelectAccounts` & `SelectPools`
  - Add a generalized unit test for all actors
- Remove `NewPersistenceModule` and an injected `Config` + `Create`
  - This improves isolation a a “injection-like” paradigm for unit testing
- Change `SetupPostgresDocker` to `SetupPostgresDockerPersistenceMod`
  - This enables more “functional” like testing by returning a persistence module and avoiding global testing variables
  - Only return once a connection to the DB has been initialized reducing the likelihood of test race conditions
- Implemented `NewReadContext` with a proper read-only context

**Secondary persistence module changes**

- Improve return values in `Commit` and `Release` (return error, add logging, etc…)
- Add `pgx.Conn` pointer to `PostgresDB`
- `s/db/conn/g` and `s/conn/tx/g` in some (not all) places where appropriate
- Make some exported variables / functions unexported for readability & access purposes
- Add a few helpers for persistence related unit testing
- Added unit tests and TODOs for handling multiple read/write contexts

## General milestone checklist
- [x] Update all the relevant CHANGELOGs
- [ ] Update all the relevant READMEs
- [ ] Update the source code tree explanation
- [ ] Add or update a state, sequence or flowchart diagram using [mermaid](https://mermaid-js.github.io/mermaid/)
- [x] Create followup milestones + issues
- [x] Document small TODO along the way

## Follow-up Work

All follow-up work is being tracked in #149

---

## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [ ] If applicable, I have made corresponding changes to related or global README
- [ ] If applicable, I have added added new diagrams using [memaid.js](https://mermaid-js.github.io)
- [ ] If applicable, I have added tests that prove my fix is effective or that my feature works

---

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local>
Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan>
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
Co-authored-by: Andrew Nguyen <amnguyen1@mail.usf.edu>
@Olshansk Olshansk moved this from Todo to In Progress in V1 Dashboard Aug 23, 2022
@Olshansk Olshansk removed their assignment Aug 27, 2022
@Olshansk Olshansk moved this from In Progress to TODO in V1 Dashboard Sep 4, 2022
@Olshansk Olshansk changed the title [Persistence] Cleanup Persistence Module [Persistence] Fix Persistence Module TODOs Sep 4, 2022
@Olshansk Olshansk moved this from TODO to Up Next in V1 Dashboard Sep 4, 2022
@Olshansk Olshansk moved this from Up Next to Rescope / decompose in V1 Dashboard Sep 5, 2022
@Olshansk Olshansk assigned Olshansk and unassigned andrewnguyen22 Sep 11, 2022
@Olshansk
Copy link
Member

@andrewnguyen22 Per the discussion last friday, I'll take on persistence in the cleanup phase. I'll submit every bullet point here (and others) as a separate PR.

Screen Shot 2022-09-11 at 4 00 23 PM

andrewnguyen22 pushed a commit that referenced this issue Sep 12, 2022
@jessicadaugherty jessicadaugherty changed the title [Persistence] Fix Persistence Module TODOs [Persistence] Fix Persistence Module TODOs - Consolidate Postgres Sep 12, 2022
@jessicadaugherty jessicadaugherty moved this from Rescope / decompose to Up Next in V1 Dashboard Sep 12, 2022
@Olshansk Olshansk moved this from Up Next to In Progress in V1 Dashboard Sep 14, 2022
@Olshansk Olshansk moved this from In Progress to In Review in V1 Dashboard Sep 19, 2022
Olshansk added a commit that referenced this issue Sep 23, 2022
## Description

Tend to tech debt in the `persistence` module related to simplifying the `PostgresContext` struct.

## Issue

Fixes #149

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other: <REPLACE_ME>

## List of changes

- Consolidates `PostgresContext` and `PostgresDB`
- Reduces scope of `Tx` and `BlockStore`
- Improve error handling in `NewFuzzTestPostgresContext`

## Testing

- [x] `make test_all`
- [ ] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [ ] If applicable, I have made corresponding changes to related local or global README
- [ ] If applicable, I have added new diagrams using [mermaid.js](https://mermaid-js.github.io)
- [ ] If applicable, I have added tests that prove my fix is effective or that my feature works
Repository owner moved this from In Review to Done in V1 Dashboard Sep 23, 2022
h5law added a commit that referenced this issue Jan 20, 2023
…t` into a shared interface (Issue #102) (#442)

## Description

This PR addresses issue #102. It consolidates `Account` and `Pool` to
use a shared interface `ProtocolAccountSchema` that generalises the
functions that both actors use.

The `ProtocolAccountSchema` interface is implemented by the
`baseProtocolAccountSchema` struct - following the same pattern laid out
by the `ProtocolActorSchema` and `baseProtocolActorSchema`. Two new
types are then created - `Account` and `Pool` these are instances of the
`baseProtocolAccountSchema` struct and are initiated with their relevant
`SQL` table names, height constraints and column names.

The functions in the `PersistenceRWContext` for both `Account` and
`Pool` utilities are now generalised to use shared logic dramatically
reducing the code footprint.

In addition to this all relevant unit tests have been updated and usages
of these functions are working correctly. Also regarding issue #149
`address []bytes` has been removed from `InsertPool`.

## Issue

Fixes #102 

## Type of change

Please mark the relevant option(s):

- [x] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Create `ProtocolAccountSchema` interface and
`baseProtocolAccountSchema` struct that implements this interface
- Create `Account` and `Pool` types that share the common functionality
of `baseProtocolAccountSchema`
- Generalise all `Account` and `Pool` functions to use shared logic
- Remove `address []byte` from `InsertPool()`
- Seperate `shared_sql.go` and `types/shared_sql.go` into
`account_shared_sql.go` and `actor_shared_sql.go` as well as
`types/account_shared_sql.go` and `types/actor_shared_sql.go`
respectively

## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`

## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist
- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core infrastructure - protocol related persistence Persistence specific changes
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants