Remove checkNotNull from the DefaultBlockStoreMeta

[LuQQiu]

In DefaultBlockStoreMeta.java, we have the following code

  @Override
  public Map<String, List<Long>> getBlockList() {
    Preconditions.checkNotNull(mBlockIdsOnTiers, "mBlockIdsOnTiers");

    return mBlockIdsOnTiers;
  }

  @Override
  public Map<BlockStoreLocation, List<Long>> getBlockListByStorageLocation() {
    Preconditions.checkNotNull(mBlockIdsOnLocations, "mBlockIdsOnLocations");

    return mBlockIdsOnLocations;
  }

The checkNotNull for the two operations which I think does not make sense. In the DefaultBlockWorkerTest, I want to add something like

  public void getStoreMeta() {
    BlockStoreMeta storeMeta = mBlockWorker.getStoreMeta();
    // TODO(lu) add back this line after refactor the StoreMeta
    // assertTrue(storeMeta.getBlockList().isEmpty());

which doesn't work since the Preconditions.checkNotNull. As a user, I would expect an empty list is returned when I get block store metadata when it doesn't have any blocks inside.

This task is not asking you to create a PR to remove the checkNotNull directly, but a request to ask you to see what's the reasons behind the checkNotNull, do they make sense. If they make sense, comment on this issue. If not, change them, make sure nothing breaks and add related unit tests.

Time estimate: one to two weeks.

[yabola]

DefaultBlockStoreMeta constructor has a parameter shouldIncludeBlockIds which means getBlockStoreMeta or getBlockStoreMetaFull. When we don't need to get full meta, mBlockIdsOnTiers and mBlockIdsOnLocations will not be initialized. Why did the previous author add checkNotNull? I guess: getBlockList() is meaningless when we don't get full metadata (it cannot be used without being initialized). This can avoid some misuse i think. null and empty is different. one is not initialized and one is that doesn't have any blocks inside. if we want to use getBlockList() ,we need to make sure that we had got full metadata (use getBlockStoreMetaFull instead of getBlockStoreMeta).

[LuQQiu]

Got it, so the CheckNotNull makes sense if getBlockMeta is used instead of getBlockMetaFull. Can you create a PR to modify the description of BlockStoreMeta.getBlockList and related methods to give clear reasons of why CheckNotNull is included and why this method is only available when blocks are included?

The current method descriptions are not clear enough for users. Thanks for all the investigation!

[yabola]

sure, I will create a PR later

[LuQQiu]

That's the JavaDoc guide that we aim to follow: https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html The Java documentation should be implementation dependent but should be complete, including boundary conditions, parameter ranges and corner cases.