Skip to content

RM_GetContextFlags provides indication that we're in a fork child#7783

Merged
oranagra merged 2 commits intoredis:unstablefrom
oranagra:is_in_fork_child
Sep 20, 2020
Merged

RM_GetContextFlags provides indication that we're in a fork child#7783
oranagra merged 2 commits intoredis:unstablefrom
oranagra:is_in_fork_child

Conversation

@oranagra
Copy link
Member

@oranagra oranagra commented Sep 10, 2020

Also if the fork child is a module fork child, reduce the log level
of sigKillChildHandler since some modules create a lot of forks and
if floods the log.

@oranagra
RM_GetContextFlags provides indication that we're in a fork child
afc6804 
Also if the fork child is a module fork child, reduce the log level
of sigKillChildHandler since some modules create a lot of forks and
if floods the log.
@oranagra oranagra requested review from guybe7 and yossigo September 10, 2020 11:41
@oranagra oranagra mentioned this pull request Sep 10, 2020
Closed
guybe7
guybe7 reviewed Sep 10, 2020
View reviewed changes
src/scripting.c
ldb.forked = (c->flags & CLIENT_LUA_DEBUG_SYNC) == 0;
if (ldb.forked) {
pid_t cp = redisFork();
pid_t cp = redisFork(CHILD_TYPE_LDB);
Copy link
Collaborator

@guybe7 guybe7 Sep 10, 2020

Choose a reason for hiding this comment

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

@oranagra typo? (LDB -> RDB)

Copy link
Member Author

@oranagra oranagra Sep 10, 2020

Choose a reason for hiding this comment

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

no.. if it was a typo it would have failed to build.. 8-)
LDB = Lua Debugger (look at the function we're inside)

@yossigo
Copy link
Collaborator

yossigo commented Sep 13, 2020

@oranagra What about backwards compatibility? RM_GetContextFlags() is not defined as an experimental API, we don't offer any simple way for a module to distinguish between a context that has no REDISMODULE_CTX_FLAGS_IS_CHILD because it's not a child vs. a one that is a child but runs an older version of Redis.

@oranagra
Copy link
Member Author

oranagra commented Sep 13, 2020

@yossigo that's an interesting concern, but it's one that we completely ignored so far, so this new flag is not special in that respect.
We also never bothered to document which API or flag was added in which version. module authors need to bisect git in order to find that out.
i suggest to merge it if we're happy with it and deal with that concern separately.

I suggest the following new API for the version / features check:

int RM_GetContextFlagsAll(); // returns an int with all supported flags turned on.
int RM_GetServerVersion(); // returns an int in the format 0x00MMmmpp; e.g. 0x00060008;

@redis/core-team @guybe7 @MeirShpilraien please share your thoughts on both the current PR and the suggestion above.

@yossigo
Copy link
Collaborator

yossigo commented Sep 13, 2020

@oranagra IIRC this did come up while discussing RM_HashSet() but I agree it's widely overlooked and can be addressed as a separate issue.

yossigo
yossigo previously approved these changes Sep 13, 2020
View reviewed changes
@MeirShpilraien
Copy link

MeirShpilraien commented Sep 13, 2020

@oranagra I agree with the new REDISMODULE_CTX_FLAGS_IS_CHILD flag, I also agree with the new suggested API but I believe we should also add a function like RM_GetContextFlagsAll to supported keyspace notification flags (maybe int RM_SupportedKeySpaceNotifications()). Though RM_SubscribeToKeyspaceEvents has a return value I think this is a better approach then start removing flags and check which is supported (also, today the implementation is that it never fails.). Today on RediSearch we check the redis version to understand if we have the load event or not.

In addition, Maybe we should also do the same for the server events API?

If you approve I will be happy to take it.

@oranagra
Copy link
Member Author

oranagra commented Sep 14, 2020

@MeirShpilraien seems fine. let's wait a day for further feedback and then will be happy if you can take it.

@oranagra oranagra added state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten labels Sep 14, 2020
@yossigo
Copy link
Collaborator

yossigo commented Sep 14, 2020

@oranagra @MeirShpilraien I'd prefer to have a dedicated issue/PR for this, and try to aim for a more generic macro to check availability of a specific flag or function.

Function checks are easy (i.e. function != NULL), but I think hiding this detail behind a macro makes sense. e.g.

RMAPI_FUNC_SUPPORTED(name)
RMAPI_CTXFLAG_SUPPORTED(name)

etc.

@MeirShpilraien
Copy link

MeirShpilraien commented Sep 14, 2020

I agree, will open another issue with a suggestion and we can continue the discussion there.

@MeirShpilraien MeirShpilraien mentioned this pull request Sep 16, 2020
Closed
@oranagra oranagra requested review from a team and yossigo September 16, 2020 14:10
@oranagra oranagra merged commit 2458e54 into redis:unstable Sep 20, 2020
@oranagra oranagra deleted the is_in_fork_child branch September 20, 2020 10:43
@oranagra oranagra mentioned this pull request Oct 26, 2020
Merged
oranagra added a commit that referenced this pull request Oct 27, 2020
@oranagra
RM_GetContextFlags provides indication that we're in a fork child (#7783
fd610ae 
)

(cherry picked from commit 2458e54)
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 4, 2020
@oranagra
RM_GetContextFlags provides indication that we're in a fork child (re…
fa749b5 
…dis#7783)
jschmieg pushed a commit to memKeyDB/memKeyDB that referenced this pull request Nov 6, 2020
@oranagra @jschmieg
RM_GetContextFlags provides indication that we're in a fork child (re…
6b20ef8 
…dis#7783)

(cherry picked from commit 2458e54)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybe7 guybe7 guybe7 left review comments

@yossigo yossigo yossigo approved these changes

@itamarhaber itamarhaber itamarhaber approved these changes

Assignees

No one assigned

Labels

state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@oranagra @yossigo @MeirShpilraien @itamarhaber @guybe7