Do not pass this as an argument

[navina]

Passing this as an argument can lead to concurrency issues.

Removing TableDataManager instance from TableUpsertMetadataManager#init interface as it is not actually used. TableUpsertMetadataManager is created by the TableDataManager. Hence, any required data can be passed in directly without exposing this.

Note: Kept the change backward compatibility , although I don't see a reason why we can't just remove it. It is an internal API.

Label: refactor

[codecov-commenter]

Codecov Report

Merging #9835 (5f92f2d) into master (e98fa6e) will decrease coverage by 6.00%.
The diff coverage is 25.00%.

@@             Coverage Diff              @@
##             master    #9835      +/-   ##
============================================
- Coverage     70.26%   64.26%   -6.01%     
- Complexity     4924     4976      +52     
============================================
  Files          1964     1913      -51     
  Lines        105055   102696    -2359     
  Branches      15907    15621     -286     
============================================
- Hits          73822    66002    -7820     
- Misses        26101    31916    +5815     
+ Partials       5132     4778     -354     

Flag	Coverage Δ
integration1	?
integration2	?
unittests1	67.96% <25.00%> (-0.01%)	⬇️
unittests2	15.81% <0.00%> (+0.02%)	⬆️

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

Impacted Files	Coverage Δ
...ata/manager/realtime/RealtimeTableDataManager.java	28.57% <0.00%> (-41.70%)	⬇️
...t/local/upsert/BaseTableUpsertMetadataManager.java	57.89% <ø> (ø)
...gment/local/upsert/TableUpsertMetadataManager.java	0.00% <0.00%> (ø)
...ocal/upsert/TableUpsertMetadataManagerFactory.java	47.05% <100.00%> (ø)
...va/org/apache/pinot/common/config/NettyConfig.java	0.00% <0.00%> (-100.00%)	⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java	0.00% <0.00%> (-100.00%)	⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java	0.00% <0.00%> (-100.00%)	⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java	0.00% <0.00%> (-100.00%)	⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java	0.00% <0.00%> (-100.00%)	⬇️
...ache/pinot/server/access/AccessControlFactory.java	0.00% <0.00%> (-100.00%)	⬇️
... and 435 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[navina]

@Jackie-Jiang can you review this PR? can you also re-trigger the CI suite? The failed integration test runs fine locally.

[Jackie-Jiang]

Suggest not changing the interface TableUpsertMetadataManager which is actually designed as a pluggable module. We want it to be able to access TableDataManager for extra configs. Both of them are table level, so there should be no concurrency issue though.

[navina]

Spoke with @Jackie-Jiang offline.
I think my suggestion was coming from a cleaner code point of view that doesn't expose functionality (which otherwise may not be necessary to different components of the system) and to improve testability.
I still think there should be very little need to access functionality from a higher level. Data access can be done without passing the entire instance. But it looks like we are a little too deep in the weeds to simplify this right now :)

Closing this and will re-visit the pattern later. Tks!