[#9009]improvement(lance-rest-server): add ITs for LRS namespace operations by mchades · Pull Request #9008 · apache/gravitino

mchades · 2025-11-03T11:58:43Z

What changes were proposed in this pull request?

add ITs for LRS namespace operations

Why are the changes needed?

Fix: #9009

Does this PR introduce any user-facing change?

no

How was this patch tested?

CI pass

jerryshao · 2025-11-04T02:09:45Z

+            AuxiliaryServiceManager.GRAVITINO_AUX_SERVICE_PREFIX
+                + AuxiliaryServiceManager.AUX_SERVICE_NAMES);
+    if (value != null && !value.isEmpty()) {
+      List<String> serviceNames = COMMA.splitToList(value);


Will this splitToList also do the trim to avoid some invisible characters like white space?

yes. Here is the definition of COMMA:

private static final Splitter COMMA = Splitter.on(",").omitEmptyStrings().trimResults();

jerryshao · 2025-11-04T02:10:48Z

+          updatedServiceNames.add(serviceName);
+        }
+      }
+      String updatedValue = String.join(",", updatedServiceNames);


If the updatedServiceNames is empty, shall we remove the config?

jerryshao · 2025-11-04T02:15:21Z

+      throws IOException {
+    if (context.ignoreLanceAuxRestService) {
+      return new HashMap<>();
+    }


You can use Collections.emptyMap().

jerryshao · 2025-11-04T02:50:31Z

+      customConfigs.put(LANCE_CONFIG_PREFIX + NAMESPACE_BACKEND_URI.getKey(), gravitinoUri);
+
+      String metalakeName = GravitinoITUtils.genRandomName("LanceRESTService_metalake");
+      customConfigs.put(LANCE_CONFIG_PREFIX + METALAKE_NAME.getKey(), metalakeName);


Shall we create the metalake here before using Lance namespace server?

My thought is that the test class can use getLanceRESTServerMetalakeName() in BaseIT to create the metalake before the test and perform cleanup afterward. If we create the metalake here, it will be difficult to clean up.

jerryshao · 2025-11-04T02:51:27Z

+        .filter(e -> !e.getKey().equalsIgnoreCase(Catalog.PROPERTY_IN_USE))
+        .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
+  }
+


Why do we remove this?

It's used to remove the in-use property from the old catalog when creating a catalog namespace with overwrite mode.

Because we use removeOldCatalogProperty and setNewCatalogProperty to achieve this, the in-use property in the catalog is reserved and cannot be used in removePropertyChange.

jerryshao · 2025-11-04T02:53:17Z

+    Assertions.assertEquals(
+        IllegalArgumentException.class.getSimpleName(),
+        exception.getErrorResponse().get().getType());
+  }


Can you add more tests like namespace not exist scenario here?

mchades · 2025-11-04T06:34:21Z

@jerryshao all comments resolved, could you plz help to review again? thx!

yuqi1129 · 2025-11-04T07:40:40Z

      miniGravitino.start();
      serverConfig = miniGravitino.getServerConfig();
    } else {
+      if (!ignoreLanceAuxRestService) {


Why can't we move the code ignoreLanceAuxRestService and ignoreRestAuxRestService to L346 or before, I noticed that the same logic is there.

yuqi1129 · 2025-11-04T07:45:41Z

+  public void startIntegrationTest() throws Exception {
+    super.ignoreLanceAuxRestService = false;
+    super.startIntegrationTest();
+    this.metalake = createMetalake(getLanceRESTServerMetalakeName());


Why don't we set a constant name before running Lance rest server?

public void startIntegrationTest() throws Exception { customConfigs.put("gravitino.lance-rest.gravitino.metalake-name", METALAKE_NAME); super.startIntegrationTest(); }

I think BaseIT should manage the configuration instead of scattering it everywhere.

BaseIT manages common configurations, and subclasses can adjust them according to the detailed test requirement. Please note SparkIT and FlinkIT, they all make some customization on startIntegrationTest.

I don't think let BaseIT choose the matelake name is good idea.

For the current implementation, when developers want to enable the LRS in IT, they just need to extend BaseIT and set ignoreLanceAuxRestService = false before tests. They don't need to care about customConfigs, so I prefer to keep the current implementation.

BTW, the automatically generated metalake name is random. Users can also retrieve it using the getLanceRESTServerMetalakeName() method. I don't see a use case that requires specifying a metalake name.

This isn't a blocker. If you only have this one comment, let's get the ball rolling and merge this. You can address it in a separate PR if needed. WDYT? @jerryshao

If you insist that They don't need to care about customConfigs and ignore the makelake name, I have no more comment here.

You can think, why don't IcebergITs and FlinkITs also add some specific configuration and functions to the BaseITs?

I don't see a use case that requires specifying a metalake name.

That is not the core point; what matters is that it's the responsibility of the subclass of BaseIT to choose and create makelake.

@yuqi1129 you can update based on this PR.

yuqi1129 · 2025-11-04T07:46:23Z

+  @AfterEach
+  public void clearMetalake() {
+    Arrays.stream(metalake.listCatalogs()).forEach(c -> metalake.dropCatalog(c, true));
+    tempDir.toFile().deleteOnExit();


You have done it in L82, it seems that this is unnecessary.

ok. keep in afterAll only

yuqi1129 · 2025-11-04T08:52:38Z

LGTM

…e operations (apache#9008) ### What changes were proposed in this pull request? add ITs for LRS namespace operations ### Why are the changes needed? Fix: apache#9009 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? CI pass

…e operations (apache#9008) add ITs for LRS namespace operations Fix: apache#9009 no CI pass

add ITs for LRS namespace operations

33d17b9

mchades changed the title ~~improvement(lance-rest-server): add ITs for LRS namespace operations~~ [#9009]improvement(lance-rest-server): add ITs for LRS namespace operations Nov 3, 2025

mchades requested review from jerryshao and yuqi1129 November 3, 2025 12:07

mchades marked this pull request as ready for review November 3, 2025 12:07

mchades added 5 commits November 3, 2025 20:43

fix IT

e91be3f

fix container dep

65ec483

fix deploy mode

c9a738d

fix dep

9b47908

fix miss

657bb49

jerryshao reviewed Nov 4, 2025

View reviewed changes

address comments

a289055

mchades requested a review from jerryshao November 4, 2025 04:56

yuqi1129 reviewed Nov 4, 2025

View reviewed changes

address comments fron yuqi

0384cf5

mchades requested a review from yuqi1129 November 4, 2025 08:21

yuqi1129 approved these changes Nov 4, 2025

View reviewed changes

jerryshao merged commit fc0f617 into apache:branch-lance-namepspace-dev Nov 4, 2025
26 checks passed

jerryshao assigned yuqi1129 Nov 4, 2025

mchades deleted the add-tests branch November 4, 2025 11:16

mchades self-assigned this Nov 4, 2025

youngyjd pushed a commit to youngyjd/gravitino that referenced this pull request Nov 21, 2025

[apache#9009]improvement(lance-rest-server): add ITs for LRS namespac…

c8a3fa8

…e operations (apache#9008) add ITs for LRS namespace operations Fix: apache#9009 no CI pass

youngyjd pushed a commit to youngyjd/gravitino that referenced this pull request Nov 21, 2025

[apache#9009]improvement(lance-rest-server): add ITs for LRS namespac…

1e22fdc

…e operations (apache#9008) add ITs for LRS namespace operations Fix: apache#9009 no CI pass

Conversation

mchades commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchades commented Nov 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuqi1129 commented Nov 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mchades commented Nov 3, 2025 •

edited

Loading