Added process status handler #3975

iulianpascalau · 2022-04-06T15:07:35Z

added process status handler in BlockProcessors and trie storage manager. This component should be able to suspend/resume the snapshot/checkpoint process while the block processor is executing blocks, either Create, Process or Commit.

sstanculeanu · 2022-04-07T11:39:38Z

factory/coreComponentsHandler_test.go

@@ -59,6 +59,8 @@ func TestManagedCoreComponents_Create_ShouldWork(t *testing.T) {
 	require.NotEqual(t, "", managedCoreComponents.ChainID())
 	require.NotNil(t, managedCoreComponents.AddressPubKeyConverter())
 	require.NotNil(t, managedCoreComponents.RoundNotifier())
+	require.NotNil(t, managedCoreComponents.ArwenChangeLocker())
+	require.NotNil(t, managedCoreComponents.ProcessStatusHandler())


require.Nil(t, managedCoreComponents.ArwenChangeLocker()) require.Nil(t, managedCoreComponents.ProcessStatusHandler())

as well at L47

sstanculeanu · 2022-04-07T11:40:25Z

process/block/shardblock.go

@@ -758,6 +762,9 @@ func (sp *shardProcessor) CreateBlock(
 		return nil, nil, process.ErrWrongTypeAssertion
 	}

+	sp.processStatusHandler.SetToBusy("shardProcessor.ProcessBlock")


Suggested change

sp.processStatusHandler.SetToBusy("shardProcessor.ProcessBlock")

sp.processStatusHandler.SetToBusy("shardProcessor.CreateBlock")

done, thanks.

sstanculeanu · 2022-04-07T12:03:12Z

statusHandler/processStatusHandler.go

+
+type processStatusHandler struct {
+	mutStatus sync.RWMutex
+	isIdle    bool


as discussed on Slack, perhaps it would be safer to have a counter here, which will be incremented/decremented on SetToBusy/SetToIdle, even though Create/Process/Commit operations are "forced" to be serialized

~~right, will do~~
Having a second thought, I would leave it as it is for one reason: in case we misused the SetToBusy/SetToIdle calls and we end up calling more SetToBusy, we will end up never creating a snapshot which is very dangerous. Instead, at the first SetToIdle call, we will unlock the snapshot process.

…stop-snapshot

ssd04

Looks good for me 👍

AdoAdoAdo · 2022-04-11T07:37:06Z

common/interface.go

+// ProcessStatusHandler defines the behavior of a component able to hold the current status of the node and
+// able to tell if the node is idle or processing/committing a block
+type ProcessStatusHandler interface {
+	SetToBusy(reason string)


can you rename?
SetBusy
SetIdle

AdoAdoAdo · 2022-04-11T07:41:13Z

epochStart/bootstrap/storageProcess.go

@@ -460,7 +460,7 @@ func (sesb *storageEpochStartBootstrap) applyCurrentShardIDOnMiniblocksCopy(meta

 	for i := range originalMiniblocksHeaders {
 		mb := originalMiniblocksHeaders[i].ShallowClone()
-		err = mb.SetSenderShardID(sesb.importDbConfig.ImportDBTargetShardID) //it is safe to modify here as mbh is passed by value
+		err = mb.SetSenderShardID(sesb.importDbConfig.ImportDBTargetShardID) // it is safe to modify here as mbh is passed by value


maybe remove comment or rephrase?
as mb is a shallow clone.

done, renamed to:

// it is safe to modify here as mb is a shallow clone

AdoAdoAdo · 2022-04-11T07:46:15Z

integrationTests/multiShard/hardFork/hardFork_test.go

@@ -80,7 +80,7 @@ func TestHardForkWithoutTransactionInMultiShardedEnvironment(t *testing.T) {
 	time.Sleep(time.Second)

 	nrRoundsToPropagateMultiShard := 5
-	/////////----- wait for epoch end period
+	// ///////----- wait for epoch end period


can you clean the slashes?
below as well.

AdoAdoAdo · 2022-04-11T09:36:56Z

trie/interface.go

@@ -46,8 +46,8 @@ type node interface {
 	getValue() []byte

 	commitDirty(level byte, maxTrieLevelInMemory uint, originDb common.DBWriteCacher, targetDb common.DBWriteCacher) error
-	commitCheckpoint(originDb common.DBWriteCacher, targetDb common.DBWriteCacher, checkpointHashes CheckpointHashesHolder, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler) error
-	commitSnapshot(originDb common.DBWriteCacher, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler) error
+	commitCheckpoint(originDb common.DBWriteCacher, targetDb common.DBWriteCacher, checkpointHashes CheckpointHashesHolder, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler, idleProvider IdleNodeProvider) error


can you check if the idleNodeProvider can be added on the receiver, instead of as parameter, as this does not change.

can be done but the resulting trie implementations will all require this idleNodeProvider:
For example, the decodeNode function will need this parameter, then, all 3 implementations of the trie node interface, the patriciaMerkleTrie, interceptedTrieNode (along with the interceptors - and maybe the trie nodes resolvers), the trie syncers and others.
This will add significant complexity when only the snapshot/checkpoint process needs this control

AdoAdoAdo · 2022-04-11T09:37:46Z

trie/interface.go

@@ -59,8 +59,8 @@ type node interface {
 }

 type snapshotNode interface {
-	commitCheckpoint(originDb common.DBWriteCacher, targetDb common.DBWriteCacher, checkpointHashes CheckpointHashesHolder, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler) error
-	commitSnapshot(originDb common.DBWriteCacher, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler) error
+	commitCheckpoint(originDb common.DBWriteCacher, targetDb common.DBWriteCacher, checkpointHashes CheckpointHashesHolder, leavesChan chan core.KeyValueHolder, ctx context.Context, stats common.SnapshotStatisticsHandler, idleProvider IdleNodeProvider) error


same as for the node interface, maybe idleNodeProvider can be added on the receiver instead,

answered above

gabi-vuls

System test passed.

iulianpascalau and others added 2 commits April 6, 2022 18:05

- added process status handler

0666bbf

Merge branch 'feat/release-2022-April' into start-stop-snapshot

f8c42d1

sstanculeanu self-requested a review April 7, 2022 08:15

sstanculeanu reviewed Apr 7, 2022

View reviewed changes

iulianpascalau added 2 commits April 8, 2022 19:02

- fix after review

11123cd

Merge remote-tracking branch 'origin/start-stop-snapshot' into start-…

2b8e4e7

…stop-snapshot

sstanculeanu previously approved these changes Apr 8, 2022

View reviewed changes

Merge branch 'feat/release-2022-April' into start-stop-snapshot

6da78e6

ssd04 self-requested a review April 11, 2022 05:59

ssd04 previously approved these changes Apr 11, 2022

View reviewed changes

BeniaminDrasovean previously approved these changes Apr 11, 2022

View reviewed changes

AdoAdoAdo requested changes Apr 11, 2022

View reviewed changes

- fix after review: renaming, fixed comments

30f5f42

iulianpascalau dismissed stale reviews from BeniaminDrasovean, ssd04, and sstanculeanu via 30f5f42 April 11, 2022 16:20

BeniaminDrasovean approved these changes Apr 12, 2022

View reviewed changes

AdoAdoAdo approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'feat/release-2022-April' into start-stop-snapshot

a8811aa

ssd04 approved these changes Apr 12, 2022

View reviewed changes

Merge branch 'feat/release-2022-April' into start-stop-snapshot

08fe56e

gabi-vuls approved these changes Apr 13, 2022

View reviewed changes

gabi-vuls merged commit 49244eb into feat/release-2022-April Apr 13, 2022

gabi-vuls deleted the start-stop-snapshot branch April 13, 2022 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added process status handler #3975

Added process status handler #3975

iulianpascalau commented Apr 6, 2022

sstanculeanu Apr 7, 2022 •

edited

iulianpascalau Apr 8, 2022

sstanculeanu Apr 7, 2022

iulianpascalau Apr 8, 2022

sstanculeanu Apr 7, 2022

iulianpascalau Apr 8, 2022 •

edited

ssd04 left a comment

AdoAdoAdo Apr 11, 2022

iulianpascalau Apr 11, 2022

AdoAdoAdo Apr 11, 2022

iulianpascalau Apr 11, 2022

AdoAdoAdo Apr 11, 2022

iulianpascalau Apr 11, 2022

AdoAdoAdo Apr 11, 2022

iulianpascalau Apr 11, 2022 •

edited

AdoAdoAdo Apr 11, 2022

iulianpascalau Apr 11, 2022

gabi-vuls left a comment

	sp.processStatusHandler.SetToBusy("shardProcessor.ProcessBlock")
	sp.processStatusHandler.SetToBusy("shardProcessor.CreateBlock")

Added process status handler #3975

Added process status handler #3975

Conversation

iulianpascalau commented Apr 6, 2022

sstanculeanu Apr 7, 2022 • edited

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

iulianpascalau Apr 8, 2022 • edited

Choose a reason for hiding this comment

ssd04 left a 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

iulianpascalau Apr 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabi-vuls left a comment

Choose a reason for hiding this comment

sstanculeanu Apr 7, 2022 •

edited

iulianpascalau Apr 8, 2022 •

edited

iulianpascalau Apr 11, 2022 •

edited