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.

Sign up for GitHub

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

Jump to bottom

api: support to query whether pd has loaded region #8749

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
+62 −21
Open

api: support to query whether pd has loaded region #8749

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c20a4f9
api: support to query whether pd has loaded region
lhy1024 Oct 28, 2024
db574ec
fix lint
lhy1024 Oct 30, 2024
64f12cc
address comments
lhy1024 Nov 1, 2024
0c50bef
address comments
lhy1024 Nov 1, 2024
9c9e931
test compatibility
lhy1024 Nov 11, 2024
bc7428b
address comments
lhy1024 Nov 12, 2024
df4ac32
refactor
lhy1024 Nov 18, 2024
d0d6875
address comments
lhy1024 Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions pkg/storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,10 @@ type coreStorage struct {
Storage
regionStorage endpoint.RegionStorage

useRegionStorage int32
regionLoaded bool
mu syncutil.Mutex
useRegionStorage int32
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that safe to switch between using or not using RegionStore in the master branch or the latest releases now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defaultUseRegionStorage is true and only check switch when it was just elected leader.

regionLoadedFromDefault bool
regionLoadedFromStorage bool
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to distinguish these two kinds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to avoid problems after ps.useRegionStorage switch

Copy link
Member

Choose a reason for hiding this comment

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

How about using string?

regionLoadedFrom: "", "etcd", "leveldb"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, we just have these two ways now

Copy link
Member

Choose a reason for hiding this comment

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

I prefer just using a single field to store this info like:

type regionSource int

const (
	unloaded regionSource = iota
	etcd
	leveldb
)

mu syncutil.RWMutex
}

// NewCoreStorage creates a new core storage with the given default and region storage.
Expand Down Expand Up @@ -133,17 +134,22 @@ func TryLoadRegionsOnce(ctx context.Context, s Storage, f func(region *core.Regi
return s.LoadRegions(ctx, f)
}

ps.mu.Lock()
defer ps.mu.Unlock()

if atomic.LoadInt32(&ps.useRegionStorage) == 0 {
return ps.Storage.LoadRegions(ctx, f)
err := ps.Storage.LoadRegions(ctx, f)
if err == nil {
ps.regionLoadedFromDefault = true
}
return err
}

ps.mu.Lock()
defer ps.mu.Unlock()
if !ps.regionLoaded {
if !ps.regionLoadedFromStorage {
if err := ps.regionStorage.LoadRegions(ctx, f); err != nil {
return err
}
ps.regionLoaded = true
ps.regionLoadedFromStorage = true
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Seems we can use *coreStorage.LoadRegions() directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't check ps.regionLoadedFromStorage

}
Expand Down Expand Up @@ -197,3 +203,14 @@ func (ps *coreStorage) Close() error {
}
return nil
}

// AreRegionsLoaded returns whether the regions are loaded.
func AreRegionsLoaded(s Storage) bool {
ps := s.(*coreStorage)
ps.mu.RLock()
defer ps.mu.RUnlock()
if atomic.LoadInt32(&ps.useRegionStorage) == 0 {
return ps.regionLoadedFromDefault
}
return ps.regionLoadedFromStorage
}
9 changes: 5 additions & 4 deletions pkg/versioninfo/versioninfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,11 @@ import (
// Status is the status of PD server.
// NOTE: This type is exported by HTTP API. Please pay more attention when modifying it.
type Status struct {
BuildTS string `json:"build_ts"`
Version string `json:"version"`
GitHash string `json:"git_hash"`
StartTimestamp int64 `json:"start_timestamp"`
BuildTS string `json:"build_ts"`
Version string `json:"version"`
GitHash string `json:"git_hash"`
StartTimestamp int64 `json:"start_timestamp"`
AreRegionsLoaded bool `json:"are_regions_loaded"`
Copy link
Member

Choose a reason for hiding this comment

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

prefer to use loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loaded bool json:"loaded"

?

Copy link
Member

Choose a reason for hiding this comment

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

yes

}

const (
Expand Down
11 changes: 7 additions & 4 deletions server/api/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package api
import (
"net/http"

"github.com/tikv/pd/pkg/storage"
"github.com/tikv/pd/pkg/versioninfo"
"github.com/tikv/pd/server"
"github.com/unrolled/render"
Expand All @@ -39,11 +40,13 @@ func newStatusHandler(svr *server.Server, rd *render.Render) *statusHandler {
// @Success 200 {object} versioninfo.Status
// @Router /status [get]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure whether it is suitable. ref #8748 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

How about /member or /health?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/members return all member info according etcd.ListEtcdMembers
/health is to query the status of each node through the /ping interface to return information about all the nodes, if we don't add a new api, we won't be able to get the result of whether the load region is complete or not.

Copy link
Member

Choose a reason for hiding this comment

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

status is more like some fixed information to me, at least for the current definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

put it under /status, which is appropriate from the name, but currently status returns compiled information.

Yes, as I said in the issue.

func (h *statusHandler) GetPDStatus(w http.ResponseWriter, _ *http.Request) {
areRegionsLoaded := storage.AreRegionsLoaded(h.svr.GetStorage())
version := versioninfo.Status{
BuildTS: versioninfo.PDBuildTS,
GitHash: versioninfo.PDGitHash,
Version: versioninfo.PDReleaseVersion,
StartTimestamp: h.svr.StartTimestamp(),
BuildTS: versioninfo.PDBuildTS,
GitHash: versioninfo.PDGitHash,
Version: versioninfo.PDReleaseVersion,
StartTimestamp: h.svr.StartTimestamp(),
AreRegionsLoaded: areRegionsLoaded,
}

h.rd.JSON(w, http.StatusOK, version)
Expand Down
Loading