[#782]refactor: restrict rss.rpc.server.type to an enum

[advancedxy]

What changes were proposed in this pull request?

1. make RPC_SERVER_TYPE an enum
2. remove ServerType definition in various places

Why are the changes needed?

This resolves #782

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs

[codecov-commenter]

Codecov Report

Merging #783 (9331499) into master (ea5a3ba) will increase coverage by 1.62%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #783      +/-   ##
============================================
+ Coverage     57.86%   59.49%   +1.62%     
- Complexity     2028     2070      +42     
============================================
  Files           298      290       -8     
  Lines         14520    12840    -1680     
  Branches       1185     1212      +27     
============================================
- Hits           8402     7639     -763     
+ Misses         5643     4771     -872     
+ Partials        475      430      -45     

Impacted Files	Coverage Δ
...apache/uniffle/coordinator/CoordinatorFactory.java	81.81% <50.00%> (-2.80%)	⬇️
...rg/apache/uniffle/server/ShuffleServerFactory.java	73.33% <50.00%> (-3.14%)	⬇️
...e/shuffle/manager/ShuffleManagerServerFactory.java	92.85% <100.00%> (-0.90%)	⬇️
.../org/apache/uniffle/common/config/RssBaseConf.java	94.53% <100.00%> (+0.48%)	⬆️
...java/org/apache/uniffle/common/rpc/ServerType.java	100.00% <100.00%> (ø)

... and 30 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[zuston]

LGTM. plz fix the CI

[smallzhongfeng]

+1 for this change

[jerqi]

Should we add some check in the line

incubator-uniffle/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

Line 234 in ea5a3ba

nettyServerEnabled = shuffleServerConf.get(ShuffleServerConf.NETTY_SERVER_PORT) >= 0;

[advancedxy]

Should we add some check in the line

incubator-uniffle/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

Line 234 in ea5a3ba

nettyServerEnabled = shuffleServerConf.get(ShuffleServerConf.NETTY_SERVER_PORT) >= 0;

I don't get your point. could you elaborate a bit more on it?

[jerqi]

Should we add some check in the line

incubator-uniffle/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

Line 234 in ea5a3ba

nettyServerEnabled = shuffleServerConf.get(ShuffleServerConf.NETTY_SERVER_PORT) >= 0;

I don't get your point. could you elaborate a bit more on it?

If rpc.server.typeisGrpcandnetty port` is greater than 0, we should throw a exception.

[advancedxy]

Should we add some check in the line

incubator-uniffle/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

Line 234 in ea5a3ba

nettyServerEnabled = shuffleServerConf.get(ShuffleServerConf.NETTY_SERVER_PORT) >= 0;

I don't get your point. could you elaborate a bit more on it?

If rpc.server.typeisGrpcandnetty port` is greater than 0, we should throw a exception.

I don't think so. If we are going to add the following constraint, users have to modify two settings to enable netty. This adds overhead to user side compared to the current way: set netty port to a non-negative one.

[jerqi]

Should we add some check in the line

incubator-uniffle/server/src/main/java/org/apache/uniffle/server/ShuffleServer.java

Line 234 in ea5a3ba

nettyServerEnabled = shuffleServerConf.get(ShuffleServerConf.NETTY_SERVER_PORT) >= 0;

I don't get your point. could you elaborate a bit more on it?

If rpc.server.typeisGrpcandnetty port` is greater than 0, we should throw a exception.

I don't think so. If we are going to add the following constraint, users have to modify two settings to enable netty. This adds overhead to user side compared to the current way: set netty port to a non-negative one.

But if it's inconsistent, it will be confusing.

[advancedxy]

But if it's inconsistent, it will be confusing.

I would remove the GRPC_NETTY type then, from the perspective of rpc, there's no such thing as both grpc and netty.

When netty is ready, we can then make netty a dedicate rpc server type.

[jerqi]

But if it's inconsistent, it will be confusing.

I would remove the GRPC_NETTY type then, from the perspective of rpc, there's no such thing as both grpc and netty.

When netty is ready, we can then make netty a dedicate rpc server type.

OK. LGTM.

[jerqi]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

[advancedxy]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

emmm, I believe the client type might not be appropriate, and it seems never used to configure it in the client side.

We may need to refactor this client type?

[jerqi]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

emmm, I believe the client type might not be appropriate, and it seems never used to configure it in the client side.

We may need to refactor this client type?

I think it's suitable. We use the GRPC and NETTY at the same time.

[advancedxy]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

emmm, I believe the client type might not be appropriate, and it seems never used to configure it in the client side.
We may need to refactor this client type?

I think it's suitable. We use the GRPC and NETTY at the same time.

I know. But for rpc type, GRPC_NETTY might not be appropriate. The client might use grpc or grpc_netty both, but it's confused with the rpc type....

The ClientType is also used as ShuffleServer tags, which might not be appropriate. The term ClientType is more like a ServerMode thing. So I think we may reconsider the term client type here.

[jerqi]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

emmm, I believe the client type might not be appropriate, and it seems never used to configure it in the client side.
We may need to refactor this client type?

I think it's suitable. We use the GRPC and NETTY at the same time.

I know. But for rpc type, GRPC_NETTY might not be appropriate. The client might use grpc or grpc_netty both, but it's confused with the rpc type....

The ClientType is also used as ShuffleServer tags, which might not be appropriate. The term ClientType is more like a ServerMode thing. So I think we may reconsider the term client type here.

I think your thought will bring more cost that users need to understand. We should simplify the rpc type.

[advancedxy]

Em ..... but the client rpc type is GRPC and GRPC_NETTY. Is it ok?

emmm, I believe the client type might not be appropriate, and it seems never used to configure it in the client side.
We may need to refactor this client type?

I think it's suitable. We use the GRPC and NETTY at the same time.

I know. But for rpc type, GRPC_NETTY might not be appropriate. The client might use grpc or grpc_netty both, but it's confused with the rpc type....
The ClientType is also used as ShuffleServer tags, which might not be appropriate. The term ClientType is more like a ServerMode thing. So I think we may reconsider the term client type here.

I think your thought will bring more cost that users need to understand. We should simplify the rpc type.

Maybe, so we need to think it carefully.

[advancedxy]

I changed the RPCServerType to ServerType, which should be matched with ClientType.

If rpc.server.typeisGrpcandnetty port` is greater than 0, we should throw a exception.

As for this suggestion, I would like to refactor the ShuffleServer to create netty stream server via ShuffleServerFactory and do validations there.

[jerqi]

I changed the RPCServerType to ServerType, which should be matched with ClientType.

If rpc.server.typeisGrpcandnetty port` is greater than 0, we should throw a exception.

As for this suggestion, I would like to refactor the ShuffleServer to create netty stream server via ShuffleServerFactory and do validations there.

Ok for me.