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

Added gRPC server. #15

Merged
merged 18 commits into from
Apr 24, 2024
Merged

Added gRPC server. #15

merged 18 commits into from
Apr 24, 2024

Conversation

cristure
Copy link

No description provided.

)

var log = logger.GetOrCreate("main")

const (
configPath = "config/config.toml"
configPath = "/home/cristu/go/src/github.com/multiversx/mx-chain-ws-connector-firehose-go/cmd/connector/config/config.toml"
Copy link

Choose a reason for hiding this comment

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

revert to relative path

Copy link
Author

Choose a reason for hiding this comment

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

reverted.

Comment on lines 75 to 81
server = factory.NewServer(cr.config.GRPC, handler)

go func() {
if err := server.Start(); err != nil {
log.Error("couldn't start grpc server", "error", err)
}
}()
Copy link

Choose a reason for hiding this comment

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

i suggest to move this part into the componet itself

Copy link
Author

Choose a reason for hiding this comment

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

moved.

Comment on lines 22 to 34
func NewServer(config config.GRPCConfig, blocksHandler process.GRPCBlocksHandler) *GRPCServer {
s := grpc.NewServer()

service := hyperOutportBlock.NewService(blocksHandler)
data.RegisterHyperOutportBlockServiceServer(s, service)
reflection.Register(s)

return &GRPCServer{s, config}
}

// Start will start the grpc server on the configured URL.
func (s *GRPCServer) Start() error {
lis, err := net.Listen("tcp", s.config.URL)
Copy link

Choose a reason for hiding this comment

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

move this to a separate component, and add the goroutine into this component, we usually use a pattern like this one here https://github.com/multiversx/mx-chain-notifier-go/blob/main/process/publisher.go#L48

Copy link
Author

Choose a reason for hiding this comment

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

done.

Copy link

Choose a reason for hiding this comment

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

add unit tests for this file

Copy link
Author

Choose a reason for hiding this comment

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

added.

@@ -23,6 +26,7 @@ func NewDataAggregator(

return &dataAggregator{
blocksPool: blocksPool,
converter: NewOutportBlockConverter(),
Copy link

Choose a reason for hiding this comment

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

pass converter as parameter and set it in factory

Copy link

Choose a reason for hiding this comment

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

i think we should put into blocks pool the outport block already converter

Comment on lines +65 to +66
shardBlock, err := da.converter.HandleShardOutportBlock(outportBlockShard)
if err != nil {
Copy link

Choose a reason for hiding this comment

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

i think it's better to have convert outport block already in the pool

Copy link
Author

Choose a reason for hiding this comment

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

that'd require several alterations. I'd rather keep it in a separate PR.

Copy link

Choose a reason for hiding this comment

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

if these interfaces are being used only in tests, it's better to move them there unexported, otherwise it might be confusing to have them in data

Copy link
Author

Choose a reason for hiding this comment

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

moved

Comment on lines 69 to 70
if cr.config.GRPC.Enable {
writer = &fakeWriter{}
Copy link

Choose a reason for hiding this comment

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

this logic should be better put in factory package

Copy link
Author

Choose a reason for hiding this comment

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

done

config/config.go Outdated

// GRPCConfig will map the gRPC server configuration
type GRPCConfig struct {
Enable bool
Copy link

Choose a reason for hiding this comment

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

since it can be grpc mode or stdout mode exclusively, i think it's better to have a CLI paramter to specify this option

Copy link
Author

Choose a reason for hiding this comment

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

done.

cmd/connector/main.go Outdated Show resolved Hide resolved
connector/connectorRunner.go Outdated Show resolved Hide resolved
connector/connectorRunner.go Outdated Show resolved Hide resolved
connector/connectorRunner.go Outdated Show resolved Hide resolved
process/fakeWriter.go Outdated Show resolved Hide resolved
process/interface.go Show resolved Hide resolved
process/dataAggregator_test.go Outdated Show resolved Hide resolved
factory/grpcServerFactory.go Outdated Show resolved Hide resolved
server/grpcServer.go Outdated Show resolved Hide resolved
s.server.GracefulStop()

default:
lis, err := net.Listen("tcp", s.config.URL)
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 use h.server.ListenAndServe() ?

Also is it needed to do it in an infinite loop?

Copy link
Author

Choose a reason for hiding this comment

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

changed.

factory/grpcServerFactory.go Outdated Show resolved Hide resolved
outportBlocksPool process.DataPool,
dataAggregator process.DataAggregator) (process.GRPCServer, process.Writer, error) {
if !enableGrpcServer {
return nil, os.Stdout, nil
Copy link

Choose a reason for hiding this comment

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

we can have a disabled component for GRPC service in case it is not enabled

Copy link
Author

Choose a reason for hiding this comment

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

changed. but didn't implement disabled.

"github.com/multiversx/mx-chain-ws-connector-template-go/testscommon"
)

func TestService_GetHyperOutportBlockByHash(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

add parallel run for tests

Copy link
Author

Choose a reason for hiding this comment

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

added.

data "github.com/multiversx/mx-chain-ws-connector-template-go/data/hyperOutportBlocks"
)

type GRPCBlocksHandlerStub struct {
Copy link

Choose a reason for hiding this comment

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

add missing dummy comments

Copy link
Author

Choose a reason for hiding this comment

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

added.

blockContainer,
protoMarshaller,
&process.ProtoMarshaller{},
Copy link

Choose a reason for hiding this comment

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

please add a separate variable for proto marshaller and rename protoMarshaller to gogoProtoMarshaller or something similar

Copy link
Author

Choose a reason for hiding this comment

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

changed, but publisher is now in a factory.

data.UnimplementedHyperOutportBlockServiceServer
}

func NewService(blocksHandler process.GRPCBlocksHandler) *Service {
Copy link

Choose a reason for hiding this comment

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

nil check for blocks handler

Copy link
Author

Choose a reason for hiding this comment

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

added.

ssd04
ssd04 previously approved these changes Apr 23, 2024
blockContainer,
protoMarshaller,
)
s, err := factory.CreateGRPCServer(cr.enableGrpcServer, cr.config.GRPC, outportBlocksPool, dataAggregator)
Copy link

Choose a reason for hiding this comment

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

please rename the one letter variable

@@ -40,4 +40,9 @@ var (
Usage: "Option for specifying db mode. Available options: `full-persister`, `import-db`, `optimized-persister`",
Value: "full-persister",
}

enableGrpcServer = cli.BoolFlag{
Copy link

Choose a reason for hiding this comment

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

do not forget to add enableGrpcServer into app cli.Flag list

Copy link
Author

Choose a reason for hiding this comment

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

added

service/hyperOutportBlock/blockService.go Show resolved Hide resolved
process/blocksPool.go Outdated Show resolved Hide resolved
@ssd04 ssd04 merged commit 99c597a into feat/hyperblock Apr 24, 2024
3 checks passed
@ssd04 ssd04 deleted the grpcServer branch April 24, 2024 10:49
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.

3 participants