Added a service Client interface and renamed the server interface #207

abjeni · 2025-03-02T15:41:34Z

Example client interface:

// Raft is the client-side API for the Raft Service
type RaftClient interface {
	RequestVote(ctx context.Context, in *raftpb.RequestVoteRequest) (resp *raftpb.RequestVoteResponse, err error)
	AppendEntries(ctx context.Context, in *raftpb.AppendEntriesRequest) (resp *raftpb.AppendEntriesResponse, err error)
	AppendEntries2(ctx context.Context, in *raftpb.AppendEntriesRequest) (resp *raftpb.AppendEntriesResponse, err error)
	InstallSnapshot(ctx context.Context, in *commonpb.Snapshot) (resp *raftpb.InstallSnapshotResponse, err error)
	CatchMeUp(ctx context.Context, in *raftpb.CatchMeUpRequest) (resp *raftpb.Empty, err error)
}

The server interface would now look like this:

// Raft is the server-side API for the Raft Service
type RaftServer interface {
	RequestVote(ctx gorums.ServerCtx, request *raftpb.RequestVoteRequest) (response *raftpb.RequestVoteResponse, err error)
	AppendEntries(ctx gorums.ServerCtx, request *raftpb.AppendEntriesRequest) (response *raftpb.AppendEntriesResponse, err error)
	AppendEntries2(ctx gorums.ServerCtx, request *raftpb.AppendEntriesRequest) (response *raftpb.AppendEntriesResponse, err error)
	InstallSnapshot(ctx gorums.ServerCtx, request *commonpb.Snapshot) (response *raftpb.InstallSnapshotResponse, err error)
	CatchMeUp(ctx gorums.ServerCtx, request *raftpb.CatchMeUpRequest) (response *raftpb.Empty, err error)
}

Fixes #178

deepsource-io · 2025-03-02T15:41:47Z

Here's the code health analysis summary for commits a29b7b1..ecb4e77. View details on DeepSource ↗.

Analysis Summary

Analyzer	Status	Summary	Link
Go	✅ Success		View Check ↗
Shell	✅ Success		View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

meling

Nice work!

There is a small snag though. The client interface must match the client-side implementation (in the generated _gorums.pb.go files); specifically, the multicast and unicast types has an extra variadic opts argument.

One way to enforce interface compliance is to declare a variable like this (could be in a separate file in the dev folder):

// enforce interface compliance
var _ ZorumsServiceClient = (*Configuration)(nil)

This will give a compile error if the generated Configuration object does not match the ZorumsServiceClient interface.

cmd/protoc-gen-gorums/dev/zorums_client_gorums.pb.go

cmd/protoc-gen-gorums/gengorums/template_client.go

cmd/protoc-gen-gorums/dev/zorums_client_gorums.pb.go

…onfiguration and Node

abjeni · 2025-03-04T11:56:12Z

I had to create two client interfaces, one for Configuration and one for Node, which looks like this:

// Raft is the client-side Configuration API for the Raft Service
type RaftConfigurationClient interface {
	RequestVote(ctx context.Context, in *raftpb.RequestVoteRequest) (resp *raftpb.RequestVoteResponse, err error)
	AppendEntries(ctx context.Context, in *raftpb.AppendEntriesRequest, f func(*raftpb.AppendEntriesRequest, uint32) *raftpb.AppendEntriesRequest) (resp *raftpb.AppendEntriesQFResponse, err error)
}

// enforce interface compliance
var _ RaftConfigurationClient = (*Configuration)(nil)

// Raft is the client-side Node API for the Raft Service
type RaftNodeClient interface {
	AppendEntries2(ctx context.Context, in *raftpb.AppendEntriesRequest) (resp *raftpb.AppendEntriesResponse, err error)
	InstallSnapshot(ctx context.Context, in *commonpb.Snapshot) (resp *raftpb.InstallSnapshotResponse, err error)
	CatchMeUp(ctx context.Context, in *raftpb.CatchMeUpRequest) (resp *raftpb.Empty, err error)
}

// enforce interface compliance
var _ RaftNodeClient = (*Node)(nil)

meling

Nice work again... We should avoid creating empty interfaces for both Node and Configuration.

cmd/protoc-gen-gorums/gengorums/template_client.go

meling · 2025-03-04T12:17:03Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+
+var clientNodeInterface = `
+{{$genFile := .GenFile}}
+{{range .Services -}}


See below for func definitions; use like this:

{{- range nodeOnlyServices .Services}}

meling · 2025-03-04T12:18:16Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+{{$service := .GoName}}
+// {{$service}} is the client-side Node API for the {{$service}} Service
+type {{$service}}NodeClient interface {
+	{{- range .Methods}}


And this:

{{- range nodeMethods .Methods}}

meling · 2025-03-04T12:18:50Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+
+var clientConfigurationInterface = `
+{{$genFile := .GenFile}}
+{{range .Services -}}


(see implementations of the helper funcs below)

meling · 2025-03-04T12:32:24Z

cmd/protoc-gen-gorums/gengorums/gorums_func_map.go

+	},
+	"isConfigurationCall": func(method *protogen.Method) bool {
+		return hasMethodOption(method, gorums.E_Multicast, gorums.E_Quorumcall, gorums.E_Correctable, gorums.E_Async)
+	},
 	"methods": func(services []*protogen.Service) (methods []*protogen.Method) {


Perhaps we can remove this one; it does not appear to be used. I don't remember why I added it, and it seems we are using .Methods instead.

meling · 2025-03-04T12:37:23Z

benchmark/benchmark_gorums.pb.go

+var _ BenchmarkConfigurationClient = (*Configuration)(nil)
+
+// Benchmark is the client-side Node API for the Benchmark Service
+type BenchmarkNodeClient interface {


We should avoid generating empty interfaces, and the corresponding interface compliance check if it is empty. See other comments for suggestions.

meling · 2025-03-04T12:43:11Z

cmd/protoc-gen-gorums/dev/zorums_client_gorums.pb.go

+)
+
+// ZorumsService is the client-side Configuration API for the ZorumsService Service
+type ZorumsServiceConfigurationClient interface {


I wonder if we want such a long name... What do you think about just calling it ZorumsServiceClient to align with gRPC's naming?

We can still use ZorumsServiceNodeClient since that's more of a special case for Gorums.

meling · 2025-03-04T12:45:39Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+	{{- range .Methods}}
+		{{- if isConfigurationCall .}}
+			{{- if isOneway .}}
+				{{- $customOut := customOut $genFile .}}


This one does not appear to be needed since $customOut isn't used in this if-branch.

meling · 2025-03-04T12:45:49Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+	{{- range .Methods}}
+		{{- if not (isConfigurationCall .)}}
+			{{- if isOneway .}}
+				{{- $customOut := customOut $genFile .}}


This one does not appear to be needed since $customOut isn't used in this if-branch.

abjeni · 2025-03-04T14:21:17Z

the qspecServices does not do what it was supposed to do. I decided to remove it since we need a QuorumSpec interface for the Configuration struct anyway.

meling

A few more changes please. Mostly doc comment improvements and interface naming. And recall to regenerate.

meling · 2025-03-04T19:44:11Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+
+var client = clientVariables + clientConfigurationInterface + clientNodeInterface
+
+// gorumsMethods returns all Gorums-specific methods, such as multicast, quorumcall, correctable, and async methods.


gorumsMethods --> configurationMethods

meling · 2025-03-04T19:45:09Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+	return s
+}
+
+// nodeOnlyServices returns all node-only services, such as services with only unicast and plain gRPC methods.


nodeOnlyServices? Or nodeServices.

meling · 2025-03-04T19:45:54Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+	return s
+}
+
+// gorumsServices returns all services that have Gorums methods.


configurationServices

meling · 2025-03-04T19:50:06Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+{{- range configurationsServices .Services}}
+	{{$service := .GoName}}
+	// {{$service}} is the client-side Configuration API for the {{$service}} Service
+	type {{$service}}ConfigurationClient interface {


// {{$service}}Client is the client interface for the {{$service}} service. type {{$service}}Client interface {

I feel that having the Configuration name be part of the interface name gives it a connotation to something we can configure.

meling · 2025-03-04T19:52:37Z

cmd/protoc-gen-gorums/gengorums/template_client.go

+{{$genFile := .GenFile}}
+{{- range nodeServices .Services}}
+{{$service := .GoName}}
+// {{$service}} is the client-side Node API for the {{$service}} Service


// {{$service}}NodeClient is the single node client interface for the {{$service}} service. type {{$service}}NodeClient interface {

(only comment change)

abjeni · 2025-03-05T12:38:13Z

I rewrote some of the comments and fixed the qspec template, now you could maybe even have multiple services without quorum calls.
renamed ServiceConfigurationClient to ConfigurationClient.

added a client interface, and renamed the client interface

972cb93

meling requested changes Mar 2, 2025

View reviewed changes

abjeni added 2 commits March 3, 2025 20:54

fixed the interface method signatures, and split the interface into C…

75fa43b

…onfiguration and Node

fix the interface test

1022297

meling requested changes Mar 4, 2025

View reviewed changes

do not create empty client interfaces

13ee5d2

meling requested changes Mar 4, 2025

View reviewed changes

abjeni and others added 2 commits March 5, 2025 14:31

fix qspec and cleanup comments

39711e7

chore(examples/storage): rebuild also the storage example

ecb4e77

meling approved these changes Mar 5, 2025

View reviewed changes

meling merged commit 4c3bde2 into master Mar 5, 2025
5 checks passed

meling deleted the Salhus/issue178/generate_ServiceNameClient_interface branch March 5, 2025 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a service Client interface and renamed the server interface #207

Added a service Client interface and renamed the server interface #207

abjeni commented Mar 2, 2025 •

edited by meling

Loading

deepsource-io bot commented Mar 2, 2025 •

edited

Loading

Analysis Summary

meling left a comment

abjeni commented Mar 4, 2025

meling left a comment

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

abjeni commented Mar 4, 2025

meling left a comment

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

meling Mar 4, 2025

abjeni commented Mar 5, 2025


		var client = clientVariables + clientConfigurationInterface + clientNodeInterface

		// gorumsMethods returns all Gorums-specific methods, such as multicast, quorumcall, correctable, and async methods.

Added a service Client interface and renamed the server interface #207

Added a service Client interface and renamed the server interface #207

Conversation

abjeni commented Mar 2, 2025 • edited by meling Loading

deepsource-io bot commented Mar 2, 2025 • edited Loading

Analysis Summary

meling left a comment

Choose a reason for hiding this comment

abjeni commented Mar 4, 2025

meling 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abjeni commented Mar 4, 2025

meling 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

abjeni commented Mar 5, 2025

abjeni commented Mar 2, 2025 •

edited by meling

Loading

deepsource-io bot commented Mar 2, 2025 •

edited

Loading