Skip to content

[revert] "[fix][broker] change name limitTime to limitTimeInSec (#19053)" - #19152

Merged
Jason918 merged 1 commit into
apache:masterfrom
michaeljmarshall:revert-19053
Jan 8, 2023
Merged

[revert] "[fix][broker] change name limitTime to limitTimeInSec (#19053)"#19152
Jason918 merged 1 commit into
apache:masterfrom
michaeljmarshall:revert-19053

Conversation

@michaeljmarshall

Copy link
Copy Markdown
Member

This reverts commit fd5037d.

Motivation

As described in #19053 (review), the commit to be reverted introduced a breaking change in a non-backwards compatible way. This is especially important because the datastructure in question is used for ser/de of zk metadata. Instead of making one-off changes to variable names, I would prefer to first establish a standard/spec, and then apply it generally to all names. Pulsar certainly has room to improve names, but we must prioritize the user experience when changing these names. We also need to make sure that we cover upgrade and downgrade scenarios so that we do not break environments.

Modifications

Verifying this change

This should be trivial.

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

This reverts a breaking change.

Documentation

  • doc-not-needed

I'll send an email to the mailing list to discuss how we should handle these in the future.

@michaeljmarshall michaeljmarshall added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use doc-not-needed Your PR changes do not impact docs labels Jan 6, 2023
@michaeljmarshall michaeljmarshall added this to the 2.12.0 milestone Jan 6, 2023
@michaeljmarshall michaeljmarshall self-assigned this Jan 6, 2023
@michaeljmarshall michaeljmarshall changed the title Revert "[fix][broker] change name limitTime to limitTimeInSec (#19053)" [revert] "[fix][broker] change name limitTime to limitTimeInSec (#19053)" Jan 6, 2023

@dave2wave dave2wave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you

@lhotari

lhotari commented Jan 7, 2023

Copy link
Copy Markdown
Member

/pulsarbot rerun-failure-checks

@codecov-commenter

codecov-commenter commented Jan 7, 2023

Copy link
Copy Markdown

Codecov Report

Merging #19152 (9716a3a) into master (4028ad3) will increase coverage by 0.09%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19152      +/-   ##
============================================
+ Coverage     47.32%   47.42%   +0.09%     
- Complexity    10731    10755      +24     
============================================
  Files           713      713              
  Lines         69672    69672              
  Branches       7482     7482              
============================================
+ Hits          32972    33041      +69     
+ Misses        32991    32922      -69     
  Partials       3709     3709              
Flag Coverage Δ
unittests 47.42% <33.33%> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...che/pulsar/broker/service/BacklogQuotaManager.java 12.39% <0.00%> (ø)
...ker/stats/prometheus/NamespaceStatsAggregator.java 0.00% <0.00%> (ø)
...sar/broker/service/persistent/PersistentTopic.java 62.10% <50.00%> (-0.68%) ⬇️
.../org/apache/pulsar/broker/admin/AdminResource.java 65.98% <100.00%> (-0.69%) ⬇️
...rvice/schema/KeyValueSchemaCompatibilityCheck.java 21.62% <0.00%> (-45.95%) ⬇️
...rg/apache/pulsar/broker/lookup/v1/TopicLookup.java 60.00% <0.00%> (-13.34%) ⬇️
...roker/service/persistent/MessageDeduplication.java 43.23% <0.00%> (-7.43%) ⬇️
...balance/impl/SimpleResourceAllocationPolicies.java 48.57% <0.00%> (-5.72%) ⬇️
...pulsar/broker/service/PulsarCommandSenderImpl.java 67.85% <0.00%> (-5.62%) ⬇️
.../service/SystemTopicBasedTopicPoliciesService.java 69.51% <0.00%> (-3.66%) ⬇️
... and 41 more

@HQebupt

HQebupt commented Jan 8, 2023

Copy link
Copy Markdown
Contributor

@StevenLuMT FYI

@Jason918
Jason918 merged commit c862356 into apache:master Jan 8, 2023
@michaeljmarshall
michaeljmarshall deleted the revert-19053 branch January 11, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants