Functions metadata compaction #7377

srkukarni · 2020-06-27T18:15:12Z

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Currently we do not compact FunctionMetadata topic. This pr adds the ability to do that

Modifications

Describe the modifications you've done.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API: (yes / no)
The schema: (yes / no / don't know)
The default values of configurations: (yes / no)
The wire protocol: (yes / no)
The rest endpoints: (yes / no)
The admin cli options: (yes / no)
Anything that affects deployment: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

…ecutor

jerrypeng · 2020-06-29T18:08:37Z

@srkukarni the reader in the FunctionMetadataTopicTailer needs to

.readCompacted(true)

srkukarni · 2020-06-29T18:10:26Z

@srkukarni the reader in the FunctionMetadataTopicTailer needs to

.readCompacted(true)

https://github.com/apache/pulsar/pull/7377/files#diff-fbc6eb611de17f87b29ba52e30bb7fcbR140

jerrypeng · 2020-06-29T18:10:49Z

...nctions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionMetaDataManager.java

-                    break;
-                default:
-                    log.warn("Received request with unrecognized type: {}", serviceRequest);
+            if (workerConfig.getCompactMetadataTopic()) {


Or should we just check if the message has a data or not?

actually prefer explicit setting.
Plus with this change, key is being set for whether you are compacting the topic or not

Is there a way for a user to that is already using functions to be able to turn compaction on? Is there a path of migration for these users?

The simplest thing maybe to change this check from

if (workerConfig.getCompactMetadataTopic()

to

if (messsage.getdata() == null)

So old messages can get processed correctly as well as the the new messages in the new format

The format of the messages are now different.

Just the version is added as part of properties right? We can still process the messages differently based on whether the data == null or not

Not just the version. We actually write function metadata when compaction is turned on. In case when it’s not compacted, we write the service request for backwards compatibility.

jerrypeng · 2020-06-30T00:55:13Z

pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/worker/WorkerConfig.java

+            category = CATEGORY_FUNC_METADATA_MNG,
+            doc = "Should the metadata topic be compacted?"
+    )
+    private Boolean compactMetadataTopic = false;


Given the current approach, this is a dangerous flag. If a user mistakenly flips the flag on. It could render their cluster corrupt. We should perhaps add more warnings for what this config with do. Instead of "compactMetadataTopic" maybe we can rename it to "useCompactedMetadataTopic"? So that what it does is more clear.

Will change the name. Although the possibility of corruption is low. Because before a leader can actually start writing, it needs to read all existing messages and the messages will not deserialize either way.

I would suggest to add some more content to the doc annotation on what the impact of flipping the flag is.

jerrypeng · 2020-06-30T00:59:48Z

...nctions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionMetaDataManager.java

+            lastMessageSeen = exclusiveLeaderProducer.newMessage()
+                    .key(key)
+                    .value(toWrite)
+                    .property("version", Long.toString(functionMetaData.getVersion()))


lets use a

private const final String

variable for "version"

jerrypeng · 2020-06-30T17:07:37Z

...nctions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionMetaDataManager.java

        try {
-            lastMessageSeen = exclusiveLeaderProducer.send(serviceRequest.toByteArray());
+            lastMessageSeen = exclusiveLeaderProducer.newMessage()
+                    .key(key)


This will add a key for both the old format and new format right? Not sure we should add a key for the old format. Perhaps, whether checking whether the message has a key or not can be used to determine whether its the old format or new format.

jerrypeng · 2020-06-30T22:38:37Z

...nctions/worker/src/main/java/org/apache/pulsar/functions/worker/FunctionMetaDataManager.java

-                    break;
-                default:
-                    log.warn("Received request with unrecognized type: {}", serviceRequest);
+            if (workerConfig.getUseCompactedMetadataTopic()) {


Should we change this check to if a key exists or not? This creates an avenue in which a existing cluster can transition to use a compacted metadata topic

I don't think thats a good idea. I would rather have worker fail here unless specifically configured to have compaction enabled or disabled.

* Function workers re-direct call update requests to the leader * Fixed test * tests pass * Working version * Fix test * Short circuit update * Fix test * Fix test * Fix tests * Added one more catch * Added one more catch * Seperated internal and external errors * Fix test * Address feedback * Do not expose updateOnLeader to functions * hide api * hide api * removed duplicate comments * Do leadership changes in function metadata manager * make the function sync * Added more comments * Throw error * Changed name * address comments * Deleted unused classes * Rework metadata manager * Working * Fix test * A better way for test * Address feedback * Added an option to compact function metadata topic * Address feedback * Incorporate feedback Co-authored-by: Sanjeev Kulkarni <sanjeevk@splunk.com>

Sanjeev Kulkarni added 30 commits June 10, 2020 18:32

Function workers re-direct call update requests to the leader

bf11f0c

Fixed test

691d054

tests pass

cfb203d

Working version

f814544

Fix test

b0a7f28

Merge remote-tracking branch 'apache/master' into functions_leader_ex…

e612318

…ecutor

Merge branch 'master' into functions_leader_executor

ea53753

Short circuit update

c02274a

Fix test

93c54e1

Fix test

fd8766e

Fix tests

b5b1752

Added one more catch

d3403c7

Added one more catch

cc3646e

Seperated internal and external errors

6b8d6d2

Fix test

aeb51ca

Address feedback

527aea2

Do not expose updateOnLeader to functions

dcc4cd2

hide api

f8f2d34

hide api

476b692

Merge branch 'master' into functions_leader_executor

ad73099

removed duplicate comments

acd999f

Do leadership changes in function metadata manager

a256bc9

make the function sync

8fc89c3

Added more comments

6f21176

Merge branch 'master' into functions_leader_executor

f381eee

Throw error

43eaf86

Changed name

c28e122

address comments

fb86d0a

Deleted unused classes

a61e674

Rework metadata manager

d719626

Sanjeev Kulkarni added 7 commits June 24, 2020 14:24

Working

3b824e9

Fix test

c5328d1

A better way for test

abed1f7

Address feedback

7bb5bc0

Merge branch 'master' into functions_metadata_compaction

25f0c67

Merge branch 'master' into functions_metadata_compaction

62ea750

Added an option to compact function metadata topic

198aca2

srkukarni added the area/function label Jun 27, 2020

srkukarni added this to the 2.7.0 milestone Jun 27, 2020

srkukarni requested review from merlimat and jerrypeng June 27, 2020 18:15

srkukarni self-assigned this Jun 27, 2020

jerrypeng reviewed Jun 29, 2020

View reviewed changes

jerrypeng reviewed Jun 30, 2020

View reviewed changes

Address feedback

a84866d

jerrypeng reviewed Jun 30, 2020

View reviewed changes

Incorporate feedback

be0936f

jerrypeng reviewed Jun 30, 2020

View reviewed changes

jerrypeng approved these changes Jun 30, 2020

View reviewed changes

srkukarni merged commit 3d94553 into apache:master Jul 1, 2020

srkukarni deleted the functions_metadata_compaction branch July 1, 2020 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functions metadata compaction #7377

Functions metadata compaction #7377

srkukarni commented Jun 27, 2020

jerrypeng commented Jun 29, 2020

srkukarni commented Jun 29, 2020

jerrypeng Jun 29, 2020 •

edited

Loading

srkukarni Jun 29, 2020

jerrypeng Jun 29, 2020

jerrypeng Jun 29, 2020

srkukarni Jun 29, 2020

jerrypeng Jun 30, 2020

srkukarni Jun 30, 2020

jerrypeng Jun 30, 2020

srkukarni Jun 30, 2020

srkukarni Jun 30, 2020

jerrypeng Jun 30, 2020

jerrypeng Jun 30, 2020

srkukarni Jun 30, 2020

srkukarni Jun 30, 2020

jerrypeng Jun 30, 2020

srkukarni Jun 30, 2020

jerrypeng Jun 30, 2020

srkukarni Jun 30, 2020

Functions metadata compaction #7377

Functions metadata compaction #7377

Conversation

srkukarni commented Jun 27, 2020

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

jerrypeng commented Jun 29, 2020

srkukarni commented Jun 29, 2020

jerrypeng Jun 29, 2020 • edited Loading

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

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

jerrypeng Jun 29, 2020 •

edited

Loading