search

winedepot / pinot

Apache Pinot (Incubating) - A realtime distributed OLAP datastore
https://pinot.apache.org
Apache License 2.0
1 stars 0 forks source link

Let Pinot startable to prefer using hostname over ip when instance id is not set by config #10

Closed xiangfu0 closed 5 years ago

xiangfu0 commented 5 years ago

The reason for this change is that, for k8s deployment, StatefulSet could keep same pod hostname but not ip, so every restart will be treated as the old instance died forever and a new instances up joining the cluster.

Ideally this should be handled/set by internal startable wrapper which is not there yet.

Ref: https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/

codecov-io commented 5 years ago

Codecov Report

Merging #10 into develop will increase coverage by 0.17%. The diff coverage is 38.09%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop      #10      +/-   ##
=============================================
+ Coverage      67.16%   67.34%   +0.17%     
  Complexity         4        4              
=============================================
  Files           1030     1030              
  Lines          50783    50795      +12     
  Branches        7100     7103       +3     
=============================================
+ Hits           34110    34209      +99     
+ Misses         14350    14242     -108     
- Partials        2323     2344      +21
Impacted Files Coverage Δ Complexity Δ
...org/apache/pinot/common/utils/CommonConstants.java 35.55% <ø> (ø) 0 <0> (ø) :arrow_down:
...ot/core/segment/index/readers/BytesDictionary.java 54.54% <0%> (ø) 0 <0> (ø) :arrow_down:
...impl/dictionary/BytesOffHeapMutableDictionary.java 75% <0%> (ø) 0 <0> (ø) :arrow_down:
.../impl/dictionary/BytesOnHeapMutableDictionary.java 73.33% <0%> (ø) 0 <0> (ø) :arrow_down:
...tion/groupby/DictionaryBasedGroupKeyGenerator.java 96.84% <100%> (ø) 0 <0> (ø) :arrow_down:
...org/apache/pinot/controller/ControllerStarter.java 60.71% <22.22%> (-1.86%) 0 <0> (ø)
.../pinot/broker/broker/helix/HelixBrokerStarter.java 71.05% <33.33%> (-1.27%) 0 <0> (ø)
...pinot/server/starter/helix/HelixServerStarter.java 54.77% <66.66%> (-0.21%) 0 <0> (ø)
...he/pinot/core/query/pruner/ValidSegmentPruner.java 57.14% <0%> (-28.58%) 0% <0%> (ø)
.../apache/pinot/core/transport/DataTableHandler.java 88% <0%> (-12%) 0% <0%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update df53919...75a8eca. Read the comment docs.

Jackie-Jiang commented 5 years ago

@fx19880617 I agree that the host name should be set by some wrapper. So I would prefer not merging this into master. Thought?

xiangfu0 commented 5 years ago

I prefer to make this a config. The reason is that if we publish a docker image and use this in kubernetes, even for stateful deployment, ip address will not be reserved during server restart, which means that server restart will always give a new instance_id.