MX-14354: snapshotless observers support #388

bogdan-rosianu · 2023-06-30T14:32:57Z

Added support for snapshotless observers. When configuring snapshotless observers, they will be used for real-time requests (if available).
Example:

the endpoint /address/erd1.. will call snapshotless observers, being a real-time request
the endpoint /hyperblock/by-nonce/.. will call regular observers, being a historical request
if no snapshotless observer is active, the requests that were meant for them will be sent to the regular observers

# Conflicts: # process/accountProcessor.go

observer/holder/nodesHolder.go

observer/baseNodeProvider.go

observer/holder/nodesHolder.go

process/scQueryProcessor.go

observer/holder/nodesHolder_test.go

sstanculeanu · 2023-09-22T07:22:32Z

observer/holder/nodesHolder.go

+	syncedNodesCache       cacheType = "syncedNodes"
+	outOfSyncNodesCache    cacheType = "outOfSyncNodes"
+	syncedFallbackCache    cacheType = "syncedFallbackNodes"
+	outOfSyncFallbackNodes cacheType = "outOfSyncFallbackNodes"


Suggested change

outOfSyncFallbackNodes cacheType = "outOfSyncFallbackNodes"

outOfSyncFallbackCache cacheType = "outOfSyncFallbackCache"

for consistency

right. renamed

sstanculeanu · 2023-09-22T07:32:45Z

observer/holder/nodesHolder.go

+func (nh *nodesHolder) getObservers(cache cacheType, shardID uint32) []*data.NodeData {
+	cacheKey := getCacheKey(cache, shardID)
+	nh.mut.RLock()
+	cachedValues, exists := nh.cache[getCacheKey(cache, shardID)]


Suggested change

cachedValues, exists := nh.cache[getCacheKey(cache, shardID)]

cachedValues, exists := nh.cache[cacheKey]

right. fixed

sstanculeanu · 2023-09-22T07:36:10Z

observer/holder/nodesHolder.go

+	if cache == syncedFallbackCache && isSynced && isFallback {
+		return true
+	}
+	if cache == outOfSyncFallbackNodes && !isSynced && isFallback {
+		return true
+	}
+	if cache == syncedNodesCache && isSynced && !isFallback {
+		return true
+	}
+	if cache == outOfSyncNodesCache && !isSynced && !isFallback {
+		return true
+	}
+
+	return false


Suggested change

if cache == syncedFallbackCache && isSynced && isFallback {

return true

}

if cache == outOfSyncFallbackNodes && !isSynced && isFallback {

return true

}

if cache == syncedNodesCache && isSynced && !isFallback {

return true

}

if cache == outOfSyncNodesCache && !isSynced && !isFallback {

return true

}

return false

return cache == syncedFallbackCache && isSynced && isFallback ||

cache == outOfSyncFallbackNodes && !isSynced && isFallback ||

cache == syncedNodesCache && isSynced && !isFallback ||

cache == outOfSyncNodesCache && !isSynced && !isFallback

Although it reduces the code size, I do not like this approach is it is more cryptic in terms of understanding the code

I would keep it as it is since it's easier to read and can be helpful for debugging (you can trace on which if it returned true

sstanculeanu · 2023-09-22T07:38:35Z

observer/holder/nodesHolder.go

+	// nodes not cached, compute the list and update the cache
+	recomputedList := make([]*data.NodeData, 0)
+	nh.mut.Lock()
+	for _, node := range nh.allNodes[shardID] {


should we consider having here one extra check, just in case the list is already recomputed in between RUnlock and Lock?

cachedValues, exists := nh.cache[getCacheKey(cache, shardID)] if exists { return cachedValues }

sure. good idea. applied the double-checked locking pattern

sstanculeanu · 2023-09-22T07:41:32Z

observer/holder/nodesHolder_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestNodesHolder_ConstructorAndGetters(t *testing.T) {


t.Parallel() on all tests?

right. added

sstanculeanu · 2023-09-22T07:42:12Z

observer/holder/nodesHolder_test.go

+func TestNodesHolder_ConstructorAndGetters(t *testing.T) {
+	nh, err := NewNodesHolder([]*data.NodeData{}, []*data.NodeData{}, data.AvailabilityAll)
+	require.Equal(t, errEmptyNodesList, err)
+	require.True(t, check.IfNil(nh))


Suggested change

require.True(t, check.IfNil(nh))

require.Nil(t, nh)

+new test for IsInterfaceNil

sstanculeanu · 2023-09-22T07:44:37Z

observer/holder/nodesHolder_test.go

+
+	nh, err = NewNodesHolder(syncedNodes, fallbackNodes, data.AvailabilityAll)
+	require.NoError(t, err)
+	require.False(t, nh.IsInterfaceNil())


Suggested change

require.False(t, nh.IsInterfaceNil())

require.NotNil(t, nh)

sstanculeanu · 2023-09-22T07:47:00Z

observer/holder/nodesHolder_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestNodesHolder_ConstructorAndGetters(t *testing.T) {


perhaps add some calls to Count() here for 100% coverage

It has been called on the concurrency test, but added here as well

sstanculeanu · 2023-09-22T08:01:02Z

observer/circularQueueNodesProvider.go

@@ -35,11 +35,11 @@ func NewCircularQueueNodesProvider(observers []*data.NodeData, configurationFile
 }

 // GetNodesByShardId will return a slice of observers for the given shard
-func (cqnp *circularQueueNodesProvider) GetNodesByShardId(shardId uint32) ([]*data.NodeData, error) {
+func (cqnp *circularQueueNodesProvider) GetNodesByShardId(shardId uint32, dataAvailability data.ObserverDataAvailabilityType) ([]*data.NodeData, error) {
 	cqnp.mutNodes.Lock()


since I also perform write operations, I would leave it as it is

sstanculeanu · 2023-09-22T08:01:08Z

observer/circularQueueNodesProvider.go

@@ -51,11 +51,11 @@ func (cqnp *circularQueueNodesProvider) GetNodesByShardId(shardId uint32) ([]*da
 }

 // GetAllNodes will return a slice containing all observers
-func (cqnp *circularQueueNodesProvider) GetAllNodes() ([]*data.NodeData, error) {
+func (cqnp *circularQueueNodesProvider) GetAllNodes(dataAvailability data.ObserverDataAvailabilityType) ([]*data.NodeData, error) {
 	cqnp.mutNodes.Lock()


since I also perform write operations, I would leave it as it is

iulianpascalau · 2023-09-19T07:31:56Z

common/options.go

@@ -111,6 +111,15 @@ type AccountQueryOptions struct {
 	HintEpoch      core.OptionalUint32
 }

+// AreHistoricalCoordinatesSet returns true if historical block coordinates are set
+func (a AccountQueryOptions) AreHistoricalCoordinatesSet() bool {


iulianpascalau · 2023-09-19T07:38:41Z

observer/baseNodeProvider.go

 	if hasBackup {
 		return []*data.NodeData{backupNode}, nil
 	}

 	return nil, ErrShardNotAvailable
 }

-func (bnp *baseNodeProvider) getSyncedNodesUnprotected() ([]*data.NodeData, error) {
+func filterNodesInShard(nodes []*data.NodeData, shardID uint32) []*data.NodeData {


I guess we do not need this if the bnp.regularNodes.GetXXXNodes() methods will return the nodes as maps: map[uint32][]*data.NodeData, where the key uint32 is the shard ID

iulianpascalau · 2023-09-19T07:42:09Z

observer/holder/nodesHolder.go

+}
+
+func copyNodes(nodes []*data.NodeData) []*data.NodeData {
+	sliceCopy := make([]*data.NodeData, 0, len(nodes))


could have used copy method. However, if we are going to refactor this implementation to hold the nodes as maps, we will still require the iteration on the keys of the map. We can then use copy for the slices

iulianpascalau · 2023-09-19T12:40:42Z

observer/holder/nodesHolder.go

+	log                   = logger.GetOrCreate("observer/holder")
+)
+
+type nodesHolder struct {


as discussed, we can greatly simplify this struct

iulianpascalau · 2023-09-22T08:34:03Z

observer/holder/nodesHolder.go

+	nh.allNodes = make(map[uint32][]*data.NodeData)
+	nh.cache = make(map[string][]*data.NodeData)
+	for _, node := range nodesWithSyncStatus {
+		shouldSkipNode := nh.availability == data.AvailabilityRecent && !node.IsSnapshotless ||


please split this in 2. You have 2 && operators along with one ||

iulianpascalau · 2023-09-22T08:41:10Z

observer/holder/nodesHolder.go

+	mapToReturn := make(map[uint32][]*data.NodeData)
+	// in the first step, consider all the nodes to be active
+	for _, node := range regularNodes {
+		node.IsSynced = true


is this ok? You change a property of the provided slice of nodes
Why not use a clone method?

right. created a slice clone function and used it

iulianpascalau · 2023-09-22T08:43:05Z

observer/holder/nodesHolder_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestNodesHolder_ConstructorAndGetters(t *testing.T) {


Let's stop reinventing new ways of writing tests, especially for complex components. So, let's have a proper test for the constructor and for all setters & getters

added a new test for the constructor. For getters, I preferred to keep the same test (split in 2: before and after update) because one getter's response is considered 'correct' if the other getter's response is also correct

iulianpascalau · 2023-09-22T08:43:42Z

observer/holder/nodesHolder_test.go

+
+	require.Equal(t, []*data.NodeData{}, nh.GetOutOfSyncFallbackNodes(0))
+	require.Equal(t, []*data.NodeData{fallbackNodes[1]}, nh.GetOutOfSyncFallbackNodes(1))
+	require.Equal(t, []*data.NodeData{}, nh.GetOutOfSyncFallbackNodes(core.MetachainShardId))


so much for "a test should test one thing"

iulianpascalau · 2023-09-22T08:50:26Z

observer/baseNodeProvider.go

@@ -507,10 +299,18 @@ func initAllNodesSlice(nodesOnShards map[uint32][]*data.NodeData) ([]*data.NodeD
 			}

 			node := nodesOnShards[shardID][i]
-			if node.IsFallback {
-				fallbackNodes = append(fallbackNodes, node)
+			if node.IsSnapshotless {


pretty ugly if & else

couldn't find a way to refactor it

iulianpascalau · 2023-09-22T08:50:43Z

process/accountProcessor.go

@@ -452,3 +463,11 @@ func (ap *AccountProcessor) IsDataTrieMigrated(address string, options common.Ac

 	return nil, ErrSendingRequest
 }
+
+func getAvailabilityBasedOnAccountQueryOptions(options common.AccountQueryOptions) data.ObserverDataAvailabilityType {


I would create a stateless component for these If's because the ifs are higher level decision-making code
The component could also have functions that will be used in nodesHolder component L58
L157
L182

iulianpascalau · 2023-09-25T06:39:55Z

observer/holder/nodesHolder.go

+func cloneNodesSlice(input []*data.NodeData) []*data.NodeData {
+	clonedSlice := make([]*data.NodeData, len(input))
+	for idx, node := range input {
+		clonedSlice[idx] = &data.NodeData{


this works but it is not future changes proof
Could have been:

clonedNodeData := *node clonedSlice[idx] = &clonedNodeData

right. done

The base branch was changed.

bogdan-rosianu added 3 commits June 30, 2023 17:32

[WIP] snapshotless observers support

bed8f52

fixes + tests

ec2c8a6

test fix

49367d4

bogdan-rosianu changed the base branch from master to rc/v1.6.0 July 5, 2023 13:18

bogdan-rosianu changed the base branch from rc/v1.6.0 to master July 5, 2023 13:18

bogdan-rosianu changed the base branch from master to rc/v1.6.0 July 5, 2023 13:28

bogdan-rosianu added 5 commits July 5, 2023 16:30

Merge branch 'rc/v1.6.0' into MX-14354-snapshotless-observers

42dbb75

# Conflicts: # process/accountProcessor.go

fixes after merge

3c114c9

Merge branch 'rc/v1.6.0' into MX-14354-snapshotless-observers

f5f5e53

fix test

e1b2a6f

fixes after system tests

29ad567

bogdan-rosianu changed the title ~~[WIP] snapshotless observers support~~ MX-14354: snapshotless observers support Sep 19, 2023

bogdan-rosianu marked this pull request as ready for review September 19, 2023 06:47

sstanculeanu self-requested a review September 19, 2023 06:52

iulianpascalau self-requested a review September 19, 2023 07:30

sstanculeanu reviewed Sep 19, 2023

View reviewed changes

bogdan-rosianu added 3 commits September 21, 2023 16:49

refactoring

a713a29

fix after review

394794c

unit test fix

d603fed

sstanculeanu reviewed Sep 22, 2023

View reviewed changes

fixes after review

f8fd10f

iulianpascalau requested changes Sep 22, 2023

View reviewed changes

bogdan-rosianu added 2 commits September 22, 2023 15:30

fixes after second review

adff3fc

fix test

62c03fa

sstanculeanu previously approved these changes Sep 22, 2023

View reviewed changes

iulianpascalau previously approved these changes Sep 25, 2023

View reviewed changes

change clone function

fcd67a4

bogdan-rosianu dismissed stale reviews from iulianpascalau and sstanculeanu via fcd67a4 September 25, 2023 07:52

sstanculeanu previously approved these changes Sep 25, 2023

View reviewed changes

iulianpascalau previously approved these changes Sep 25, 2023

View reviewed changes

bogdan-rosianu changed the base branch from rc/v1.6.0 to feat/snapshotless-observer-support September 26, 2023 10:52

sstanculeanu approved these changes Sep 26, 2023

View reviewed changes

iulianpascalau approved these changes Sep 26, 2023

View reviewed changes

bogdan-rosianu merged commit 8f0c5d9 into feat/snapshotless-observer-support Sep 26, 2023
7 checks passed

iulianpascalau deleted the MX-14354-snapshotless-observers branch September 26, 2023 11:28

	outOfSyncFallbackNodes cacheType = "outOfSyncFallbackNodes"
	outOfSyncFallbackCache cacheType = "outOfSyncFallbackCache"

	cachedValues, exists := nh.cache[getCacheKey(cache, shardID)]
	cachedValues, exists := nh.cache[cacheKey]

MX-14354: snapshotless observers support #388

MX-14354: snapshotless observers support #388

Conversation

bogdan-rosianu commented Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdan-rosianu commented Jun 30, 2023 •

edited

Loading